Copilot commented on code in PR #3410:
URL: https://github.com/apache/tika/pull/3410#discussion_r4363691495
##########
tika-eval/tika-eval-app/src/main/resources/profile-reports.xml:
##########
@@ -36,6 +36,8 @@
end
from containers c
</sql>
+ <sql>create index pc_container_id on pipes_class (container_id)</sql>
Review Comment:
`pc_container_id` is created without `IF NOT EXISTS` (while
`ee_container_id` uses it). If the `before` block can be executed on an
existing DB (e.g., reruns, partial reruns, or a previous failed run), this can
fail with “index already exists” and stop report generation. Make index
creation idempotent by adding `if not exists` (or explicitly `drop index if
exists` before creating) for `pc_container_id` as well.
##########
tika-eval/tika-eval-app/src/main/resources/comparison-reports.xml:
##########
@@ -51,6 +53,8 @@
end
from containers c
</sql>
+ <sql>create index pcb_container_id on pipes_class_b (container_id)</sql>
+ <sql>create index if not exists eeb_container_id on extract_exceptions_b
(container_id)</sql>
Review Comment:
`pca_container_id`/`pcb_container_id` are created without `IF NOT EXISTS`,
which can cause reruns on an existing DB to fail when the index already exists
(especially since the paired `ee*` indexes are explicitly idempotent). Align
these to be idempotent as well (add `if not exists` or drop-before-create).
##########
tika-eval/tika-eval-app/src/test/java/org/apache/tika/eval/app/ComparerBatchTest.java:
##########
@@ -115,6 +116,19 @@ public void testReportOnPreLedgerDb() throws Exception {
assertTrue(summary.contains("## Overview"), summary);
}
+ /** Without these the crash-ledger joins scan pipes_class per exception
row: 9 h at 339k rows. */
+ @Test
+ public void testPipesClassJoinsUseIndexes() throws Exception {
+ for (String side : new String[]{"a", "b"}) {
+ String plan = one("explain select count(1) from
extract_exceptions_" + side + " e " +
+ "join pipes_class_" + side + " p on p.container_id =
e.container_id");
+ assertTrue(plan.contains("PC" + side.toUpperCase(Locale.ROOT) +
"_CONTAINER_ID"), plan);
+ plan = one("explain select 1 from containers c where not exists " +
+ "(select 1 from extract_exceptions_" + side + " e where
e.container_id = c.container_id)");
+ assertTrue(plan.contains("EE" + side.toUpperCase(Locale.ROOT) +
"_CONTAINER_ID"), plan);
Review Comment:
Same concern as above: matching `EXPLAIN` output for exact index names is
likely to be flaky across H2 versions/optimizer changes and may fail even when
indexes are correctly created. Consider replacing/augmenting these with
assertions against `INFORMATION_SCHEMA` to confirm index existence on
`container_id` for `pipes_class_{a,b}` and `extract_exceptions_{a,b}`, and
avoid hard-coding the `EXPLAIN` string format.
##########
tika-eval/tika-eval-app/src/test/java/org/apache/tika/eval/app/ProfilerBatchTest.java:
##########
@@ -157,6 +157,12 @@ public void testReportsRun() throws Exception {
ResultsReporter.main(new String[]{"-db",
DB.toAbsolutePath().toString(), "-rd", reportsDir.toString()});
assertTrue(Files.isRegularFile(reportsDir.resolve("exceptions/extract_exceptions_by_pipes_status.xlsx")));
assertTrue(Files.isRegularFile(reportsDir.resolve("exceptions/crash_status_extract_present.xlsx")));
+ String plan = getSingleResult("explain select count(1) from
extract_exceptions e " +
+ "join pipes_class p on p.container_id = e.container_id");
+ assertTrue(plan.contains("PC_CONTAINER_ID"), plan);
+ plan = getSingleResult("explain select 1 from containers c where not
exists " +
+ "(select 1 from extract_exceptions e where e.container_id =
c.container_id)");
+ assertTrue(plan.contains("EE_CONTAINER_ID"), plan);
Review Comment:
These assertions rely on H2 optimizer/`EXPLAIN` output containing the
specific index name strings. That’s brittle across H2 versions and data sizes
(the optimizer may choose a table scan even when an index exists, and `EXPLAIN`
formatting/wording can change). A more stable test is to assert the indexes
exist via `INFORMATION_SCHEMA` (e.g., `INFORMATION_SCHEMA.INDEXES` /
`INDEX_COLUMNS`), and (if you still want a usage check) assert more loosely on
`EXPLAIN` (e.g., presence of `INDEX` on the relevant table) rather than the
concrete index name.
##########
tika-eval/tika-eval-app/src/main/resources/comparison-reports.xml:
##########
@@ -37,6 +37,8 @@
end
from containers c
</sql>
+ <sql>create index pca_container_id on pipes_class_a (container_id)</sql>
+ <sql>create index if not exists eea_container_id on extract_exceptions_a
(container_id)</sql>
Review Comment:
`pca_container_id`/`pcb_container_id` are created without `IF NOT EXISTS`,
which can cause reruns on an existing DB to fail when the index already exists
(especially since the paired `ee*` indexes are explicitly idempotent). Align
these to be idempotent as well (add `if not exists` or drop-before-create).
--
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]
