Vladsz83 commented on code in PR #13668:
URL: https://github.com/apache/ignite/pull/13668#discussion_r4237772208
##########
modules/calcite/pom.xml:
##########
@@ -81,6 +81,13 @@
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
Is it necessary to add? I tried to removed. Th project is built normally,
some related tests tun. `CalciteOnlyNodeIntegrationTest` works locally without
it. Tried to remove Ignite's libs from the .m2.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -1019,6 +1026,23 @@ public boolean moduleEnabled() {
return indexingEnabled() || dfltQryEngine != null;
}
+ /**
+ * @param engineName Query engine name.
+ * @return {@code True} if a query engine with the given name can be
selected by the {@code QUERY_ENGINE} hint or
Review Comment:
Up to you. If we refer to a hint like `{@code QUERY_ENGINE}`, we might refer
to the property in the same way.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -198,6 +198,10 @@ public class GridQueryProcessor extends
GridProcessorAdapter {
Pattern.compile("QUERY_ENGINE[\\s]*\\([\\s]*'([a-z0-9]+)'[\\s]*\\)",
CASE_INSENSITIVE);
+ /** Error message for the features that require the H2 query engine when
it is not on the classpath. */
+ private static final String INDEXING_DISABLED_MSG = "Failed to execute
query because indexing is disabled " +
Review Comment:
Suggestion: `because indexing is disabled` -> `because the indexing is
disabled`
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -472,12 +472,14 @@ void executeNative(String sql, SqlCommand cmd,
JdbcThinStatement stmt) throws SQ
+ cliIo.igniteVersion() + ']', INTERNAL_ERROR);
}
- streamState = new StreamState((SqlSetStreamingCommand)cmd,
cliIo);
+ StreamState streamState0 = new StreamState(cmd0, cliIo);
Review Comment:
How can I test this change? Any test? `CalciteOnlyNodeIntegrationTest `
seems to work without it.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -198,6 +198,10 @@ public class GridQueryProcessor extends
GridProcessorAdapter {
Pattern.compile("QUERY_ENGINE[\\s]*\\([\\s]*'([a-z0-9]+)'[\\s]*\\)",
CASE_INSENSITIVE);
+ /** Error message for the features that require the H2 query engine when
it is not on the classpath. */
+ private static final String INDEXING_DISABLED_MSG = "Failed to execute
query because indexing is disabled " +
Review Comment:
Check `checkxModuleEnabled()` pls.
I don't like `x` in the name. But the question is can we join/reuse the
messages somehow?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -562,9 +566,12 @@ private void initQueryEngines() throws
IgniteCheckedException {
// If indexing is disabled, try to find any query engine in
components.
if (!indexingEnabled()) {
for (GridComponent cmp : ctx.components()) {
- if (cmp instanceof QueryEngine) {
- qryEngines = new QueryEngine[] {(QueryEngine)cmp};
- dfltQryEngine = (QueryEngine)cmp;
+ if (cmp instanceof QueryEngine &&
((QueryEngine)cmp).config() != null) {
Review Comment:
Suggestion: use the pattern variable:
`if (cmp instanceof QueryEngine && ((QueryEngine)cmp).config() != null)`
->
`if (cmp instanceof QueryEngine qryEngine && ((QueryEngine)cmp).config() !=
null)`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -1019,6 +1026,23 @@ public boolean moduleEnabled() {
return indexingEnabled() || dfltQryEngine != null;
}
+ /**
+ * @param engineName Query engine name.
+ * @return {@code True} if a query engine with the given name can be
selected by the {@code QUERY_ENGINE} hint or
+ * by the client connection property.
+ */
+ public boolean queryEngineConfigured(String engineName) {
+ if (qryEnginesCfg == null)
+ return false;
+
+ for (QueryEngineConfigurationEx cfg : qryEnginesCfg) {
Review Comment:
Also suggestion:
` return qryEnginesCfg != null &&
Stream.of(qryEnginesCfg).anyMatch(qCfg ->
qCfg.engineName().equalsIgnoreCase(engineName));
`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -562,9 +566,12 @@ private void initQueryEngines() throws
IgniteCheckedException {
// If indexing is disabled, try to find any query engine in
components.
if (!indexingEnabled()) {
for (GridComponent cmp : ctx.components()) {
- if (cmp instanceof QueryEngine) {
- qryEngines = new QueryEngine[] {(QueryEngine)cmp};
- dfltQryEngine = (QueryEngine)cmp;
+ if (cmp instanceof QueryEngine &&
((QueryEngine)cmp).config() != null) {
+ QueryEngine qryEngine = (QueryEngine)cmp;
+
+ qryEngines = new QueryEngine[] {qryEngine};
+ this.qryEnginesCfg = new QueryEngineConfigurationEx[]
{qryEngine.config()};
Review Comment:
Let's put this below `dfltQryEngine = qryEngine;`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -198,6 +198,10 @@ public class GridQueryProcessor extends
GridProcessorAdapter {
Pattern.compile("QUERY_ENGINE[\\s]*\\([\\s]*'([a-z0-9]+)'[\\s]*\\)",
CASE_INSENSITIVE);
+ /** Error message for the features that require the H2 query engine when
it is not on the classpath. */
+ private static final String INDEXING_DISABLED_MSG = "Failed to execute
query because indexing is disabled " +
+ "(consider adding module " + INDEXING.module() + " to classpath or
moving it from 'optional' to 'libs' folder).";
Review Comment:
Suggestion `module " + INDEXING.module() + " to classpath` -> `module '" +
INDEXING.module() + "' to classpath`
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -472,12 +472,14 @@ void executeNative(String sql, SqlCommand cmd,
JdbcThinStatement stmt) throws SQ
+ cliIo.igniteVersion() + ']', INTERNAL_ERROR);
}
- streamState = new StreamState((SqlSetStreamingCommand)cmd,
cliIo);
+ StreamState streamState0 = new StreamState(cmd0, cliIo);
Review Comment:
Suggestion: let's rename `cmd0` to smth. like `streamingCmd`.
Also above:
`boolean newVal = ((SqlSetStreamingCommand)cmd).isTurnOn();` -> `boolean
newVal = cmd0.isTurnOn()`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheProxyImpl.java:
##########
@@ -891,10 +891,16 @@ private void validate(Query qry) {
throw new CacheException("Indexing is disabled for cache: " +
cacheName +
". Use setIndexedTypes or setTypeMetadata methods on
CacheConfiguration to enable.");
- if (!ctx.kernalContext().query().moduleEnabled() &&
- (qry instanceof SqlQuery || qry instanceof SqlFieldsQuery || qry
instanceof TextQuery))
+ if (!ctx.kernalContext().query().moduleEnabled() && qry instanceof
SqlFieldsQuery) {
throw new CacheException("Failed to execute query. Add module
'ignite-indexing' to the classpath " +
- "of all Ignite nodes or configure any query engine.");
+ "of all Ignite nodes or configure any query engine.");
+ }
+
+ if ((qry instanceof SqlQuery || qry instanceof TextQuery) &&
!ctx.kernalContext().query().indexingEnabled()) {
+ throw new CacheException("Failed to execute query. " +
qry.getClass().getSimpleName() + " is supported " +
Review Comment:
Up to you. Maybe we should finally rename `indexing` to `h2`/`h2engine`
within the internals.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheProxyImpl.java:
##########
@@ -891,10 +891,16 @@ private void validate(Query qry) {
throw new CacheException("Indexing is disabled for cache: " +
cacheName +
". Use setIndexedTypes or setTypeMetadata methods on
CacheConfiguration to enable.");
- if (!ctx.kernalContext().query().moduleEnabled() &&
- (qry instanceof SqlQuery || qry instanceof SqlFieldsQuery || qry
instanceof TextQuery))
+ if (!ctx.kernalContext().query().moduleEnabled() && qry instanceof
SqlFieldsQuery) {
throw new CacheException("Failed to execute query. Add module
'ignite-indexing' to the classpath " +
- "of all Ignite nodes or configure any query engine.");
+ "of all Ignite nodes or configure any query engine.");
+ }
+
+ if ((qry instanceof SqlQuery || qry instanceof TextQuery) &&
!ctx.kernalContext().query().indexingEnabled()) {
+ throw new CacheException("Failed to execute query. " +
qry.getClass().getSimpleName() + " is supported " +
Review Comment:
Suggestion: `query. " + qry.getClass().getSimpleName() + " is` -> `query. '"
+ qry.getClass().getSimpleName() + "' is`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/GridQueryProcessor.java:
##########
@@ -1019,6 +1026,23 @@ public boolean moduleEnabled() {
return indexingEnabled() || dfltQryEngine != null;
}
+ /**
+ * @param engineName Query engine name.
+ * @return {@code True} if a query engine with the given name can be
selected by the {@code QUERY_ENGINE} hint or
+ * by the client connection property.
+ */
+ public boolean queryEngineConfigured(String engineName) {
+ if (qryEnginesCfg == null)
Review Comment:
Suggestion: If `qryEnginesCfg` can be `null`, let's put associated
`@Nullable` around. If can be empty, let's use `F.isEmpty()`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheProxyImpl.java:
##########
@@ -891,10 +891,16 @@ private void validate(Query qry) {
throw new CacheException("Indexing is disabled for cache: " +
cacheName +
". Use setIndexedTypes or setTypeMetadata methods on
CacheConfiguration to enable.");
- if (!ctx.kernalContext().query().moduleEnabled() &&
- (qry instanceof SqlQuery || qry instanceof SqlFieldsQuery || qry
instanceof TextQuery))
+ if (!ctx.kernalContext().query().moduleEnabled() && qry instanceof
SqlFieldsQuery) {
throw new CacheException("Failed to execute query. Add module
'ignite-indexing' to the classpath " +
- "of all Ignite nodes or configure any query engine.");
+ "of all Ignite nodes or configure any query engine.");
+ }
+
+ if ((qry instanceof SqlQuery || qry instanceof TextQuery) &&
!ctx.kernalContext().query().indexingEnabled()) {
+ throw new CacheException("Failed to execute query. " +
qry.getClass().getSimpleName() + " is supported " +
+ "by the H2 query engine only, add module 'ignite-indexing' to
the classpath of all Ignite nodes" +
Review Comment:
Suggestion: `only, add module 'ignite-indexing'` -> `only. Add module
'ignite-indexing'` or `To proceed, add module 'ignite-indexing'`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryEngine.java:
##########
@@ -81,4 +81,9 @@ List<FieldsQueryCursor<List<?>>> queryBatched(
String qry,
List<Object[]> batchedParams
) throws IgniteSQLException;
+
+ /** @return Configuration of the engine. */
+ default QueryEngineConfigurationEx config() {
Review Comment:
`@Nullable` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
