NSAmelchev commented on PR #13685:
URL: https://github.com/apache/ignite/pull/13685#issuecomment-6172652558
Please take a look at the AI review below.
### Blockers
1. **`SQL/sql-calcite.adoc:1326`: "If the application depends on
`ignite-indexing`, nothing changes" is wrong.**
- Scenario: an application that depends on `ignite-indexing` starts a
thick client or an embedded server, so H2 is
the default engine there. The servers come from the binary distribution
and have Calcite only.
- The server cannot read H2 messages: `UnknownMessageException: Invalid
message type: -33`.
- The communication session is closed and reopened in a loop. Put,
compute and PME to that node hang, the failure
handler does not fire, and nothing is checked on join. Reproduced.
- "All server nodes ... same set of engine modules" does not cover client
nodes.
- Fix: rewrite the bullet and add a WARNING: an engine module present on
any node, including a thick client, must be
on all server nodes, otherwise these symptoms appear. A check on join
is a separate ticket.
2. **`SQL/JDBC/jdbc-client-driver.adoc:25`: with `libs/ignite-calcite` the
JDBC Client Driver fails on every query.**
- `JdbcQueryTask.java:181` and `JdbcQueryMultipleStatementsTask.java:139`
cast the cursor to `QueryCursorImpl`, but
Calcite returns `ListFieldsQueryCursor`, so the result is a
`ClassCastException`.
- Streaming (`JdbcConnection.java:640`) requires `ignite-indexing` on the
driver side.
- Fix: state "H2 only: `ignite-indexing` on the driver and on all server
nodes; use the thin driver with Calcite".
The cast is a separate ticket: `ListFieldsQueryCursor` already
implements `QueryCursorEx`.
3. **`perf-and-troubleshooting/sql-tuning.adoc:470` and
`SQL/sql-calcite.adoc:1318`: "the on-heap row cache is H2-only"
is wrong.**
- `IndexRowCache` lives in core and is built into
`InlineIndexTree.createIndexRow` (`:355-372`). It is registered
when `moduleEnabled()` is true, so Calcite index scans go through it.
- A reader will conclude that the heap growth warning does not apply to
Calcite.
- Fix: remove the NOTE and drop the row cache from the list.
4. **Selecting an engine by name is described incorrectly.**
How the code works: `queryEngine`, `QUERY_ENGINE` and the hint work only
if the engine is listed in
`queryEnginesConfiguration`. The only exception is `calcite` on a node
where it is the only engine
(`GridQueryProcessor.java:557-579, 1034-1044`). As a result:
- `SQL/sql-calcite.adoc:48`, "To enable engine, add
`IndexingQueryEngineConfiguration`": this configuration does not
enable the engine. The class exists only in `ignite-indexing`, so the
XML does not load without the module. Taken
literally, it makes Calcite non-selectable.
- `SQL/JDBC/jdbc-driver.adoc:87`,
`SQL/ODBC/connection-string-dsn.adoc:79`: "or the only engine on the server
classpath" holds for `calcite` only.
- `SQL/sql-calcite.adoc:1343-1350`, `SQL/JDBC/jdbc-driver.adoc:696`:
- "Enable the H2 engine" does not fix `Not found configuration for
query engine: h2`;
- the hint on such a node gives `Query engines not configured, but
specified engine: h2`, which is missing from
the table;
- the COPY advice to use `queryEngine=h2` breaks the connection, while
COPY without that parameter would work.
- `:1328` contradicts `:1334`. `INDEXING_DISABLED_MSG` covers more cases,
and a node without any engine returns
a different text through the cache API, `NO_QRY_ENGINE_MSG`.
- Fix: describe one rule in "Configuring Query Engines", reference it
from JDBC/ODBC and the table, correct the
remedies, add the missing row.
5. **The quick start fails on the default distribution.** Outside the diff,
so a separate PR is fine, but before
the release.
- `quick-start/sql.adoc:68`, `tools/sqlline.adoc:191`: `LONG` and
`affinityKey=city_id` fail. With `BIGINT` and
`affinity_key` it works (verified by a run).
- The same in `code-snippets/python/sql.py` (`INT(11)`) and
`SQL/ODBC/querying-modifying-data.adoc:64,73`.
- The Node.js example in `thin-clients/nodejs-thin-client.adoc:206` uses
`SqlQuery`.
### Nice to have
1. **`SQL/sql-calcite.adoc:1311-1313`,
`key-value-api/using-cache-queries.adoc:25`.** SqlQuery and TextQuery do not
depend on the selected engine.
- They need `ignite-indexing` on the node that runs the query, including
a thick client, not "on every server node".
- `SqlQuery` runs on the default engine: with Calcite as the default,
`SqlQuery("H2VERSION() ...")` fails.
- Better to move this into a separate paragraph.
2. **queryParallelism** (`SQL/sql-calcite.adoc:1318`,
`perf-and-troubleshooting/sql-tuning.adoc:349`,
`configuring-caches/configuration-overview.adoc:88`).
- Calcite runs a fragment in one thread per node, but the index is still
split into N segments that are then
merged (`InlineIndexImpl.java:148-168`).
- "parallelizes by splitting the plan into fragments" does not describe a
replacement for queryParallelism.
- State "keep 1 with Calcite". In the configuration table, mark the
parameter as H2-only and fix the
`queryPrallelism` typo.
3. **`SQL/distributed-joins.adoc`.**
- `:66`: there is no broadcast of the smaller partitioned side in the
code. The actual options
(`AbstractIgniteJoin.java:139-205`) are: collect on one node, rehash
one or both sides, `IgniteTrimExchange`
for REPLICATED. There is no hint to choose the distribution.
- `:22`: "(the default)" needs a caveat: with both modules and no
configuration, the default engine is H2.
4. **`session-context.adoc:41`.** "In such cases" now refers to H2, but the
zero-argument constructor is actually
required by Calcite (`ExecutionContext.java:487-500`).
5. **`monitoring-metrics/system-views.adoc:597`.** The plan history is one
per node, and its default depends on the
node's default engine (`RunningQueryManager.java:210-215`). "To enable it
for H2" in fact enables it for all engines.
6. **`replicatedOnly`** (`SQL/ODBC/connection-string-dsn.adoc:95`,
`SQL/sql-calcite.adoc:1317`). The flag is
deprecated and ignored by both engines since 2.8.
7. **`perf-and-troubleshooting/sql-tuning.adoc`.**
- Both engines accept `EXPLAIN PLAN FOR`, but Calcite rejects `EXPLAIN
SELECT`. The Calcite plan is a single row
in the `PLAN` column.
- `:133`: the insertion separated "In other words" from the definition of
the flag.
- `:153`: "smaller table first" is an H2 heuristic; with `IgniteHashJoin`
it is the other way round.
8. **`SQL/sql-calcite.adoc:182`: "DDL ... compliant with H2" is wrong.** On
Calcite, `LONG`, `INT(11)`,
`affinityKey`/`cachegroup`, `PARALLELISM`, `PK_INLINE_SIZE` and `SPATIAL`
do not work.
9. **Upgrade Notes (`SQL/sql-calcite.adoc:1323`).**
- An XML configuration with `IndexingQueryEngineConfiguration` from the
old example does not load without the module.
- State the version: "Starting with 2.19" (`:17`).
- Calcite does not apply `sql.disabledFunctions`, so a user-defined
function ban silently stops working (verified
by a run).
- `installation/upgrades.adoc` does not mention the engine switch.
10. **The H2-only list (`SQL/sql-calcite.adoc:1311-1319`) and
`setup.adoc:210` ("Required only for the features
listed").** Missing:
- JDBC partition awareness: on Calcite the query silently goes to a
random node;
- `SPATIAL`/`GEOMETRY`;
- GROUP_CONCAT with `collocated`
(`sql-reference/aggregate-functions.adoc:156`);
- Spring Data and Spark;
- H2 functions generated by LINQ (`net-specific/net-linq.adoc`).
11. **The H2-only notes are gone after "Review fixes"** for `COPY`/`SET
STREAMING` in
`sql-reference/operational-commands.adoc` and for ODBC streaming
(`SQL/ODBC/querying-modifying-data.adoc:465`).
The PR description still promises them, as well as the notes in hints,
window-functions and transactions; it needs
an update.
12. **Docker (`installation/installing-using-docker.adoc:189-205`).** The
Dockerfile is in this repo
(`deliveries/docker`) and takes the distribution as is; `OPTION_LIBS` is
empty by default. The image will run on
Calcite, while the page names `ignite-indexing` as a default module.
13. **.NET (`net-specific/net-deployment-options.adoc:79`).** The NuGet
package
(`modules/platforms/dotnet/build.ps1:87,174-178`) contains only
indexing, h2 and lucene, without Calcite, so NuGet
nodes run on H2. Either add a caveat to the docs or file a ticket for
`build.ps1`.
14. **`modules/calcite/README.txt`** (it ends up in `libs/ignite-calcite/`)
still says to move the module from optional
and to keep `ignite-indexing`.
15. **The removed `SQL/sql-tuning`** page has been published since 2020. It
needs a redirect in the ignite-website
`.htaccess` (`docs/README.adoc:129-135`).
16. **`SQL/sql-introduction.adoc:25`:** the sentence has no verb, and "For
more information about see" is broken.
17. **Minor:** `QueryEnginesConfiguration` is capitalized; the WARNING
"cache is unlimited" is outdated
(there is `sqlOnheapCacheMaxSize`).
--
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]
