[
https://issues.apache.org/jira/browse/TIKA-4997?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131118#comment-18131118
]
ASF GitHub Bot commented on TIKA-4997:
--------------------------------------
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).
> Add indices for new fields in tika-eval
> ---------------------------------------
>
> Key: TIKA-4997
> URL: https://issues.apache.org/jira/browse/TIKA-4997
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Trivial
>
> index container_id on pipes_class* and extract_exceptions* – this is a
> problem if there are a lot of missing files.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
