perf(sql): disable column pre-touch in parallel filter by puzpuzpuz · Pull Request #6280 · questdb/questdb

puzpuzpuz · 2025-10-16T10:20:55Z

Disables column pre-touch in parallel filters. When the pre-touch enabled, each thread was reading selected column values from the disk for each filtered row. While this improved query performance in certain scenarios, in many other scenarios it had a negative effect on the query execution time. That's why this patch disables column pre-touch.

The cairo.sql.parallel.filter.pretouch.enabled configuration property is marked as deprecated which means that it'll be ignored by the server. From now on, column pre-touch has to be enabled per-query, with a new SQL hint:

SELECT /*+ ENABLE_PRE_TOUCH(trades) */ symbol, side, timestamp
FROM trades
WHERE symbol = 'BTC-USDT' and timestamp in today();

coderabbitai · 2025-10-16T10:21:32Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR replaces a public configuration flag for SQL parallel filter pre-touch with a query-hint-based system. It removes sqlParallelFilterPreTouchEnabled from configuration classes, deprecates the corresponding property key, and introduces ENABLE_PRE_TOUCH SQL hints. Pre-touch toggles are removed from filter and join factories, which now respect the hint instead. Async filter factories are updated to accept an enablePreTouch parameter propagated from the code generator.

Changes

Cohort / File(s)	Summary
Configuration API Removal `core/src/main/java/io/questdb/PropServerConfiguration.java`, `core/src/main/java/io/questdb/cairo/CairoConfiguration.java`, `core/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.java`, `core/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.java`	Removed public accessor methods `isSqlParallelFilterPreTouchEnabled()` and deprecated `PropertyKey.CAIRO_SQL_PARALLEL_FILTER_PRETOUCH_ENABLED`
Execution Context Changes `core/src/main/java/io/questdb/griffin/SqlExecutionContext.java`, `core/src/main/java/io/questdb/griffin/SqlExecutionContextImpl.java`	Removed public methods for column pre-touch querying and mutation: `isColumnPreTouchEnabled()`, `isColumnPreTouchEnabledOverride()`, and their setters
SQL Hints Framework `core/src/main/java/io/questdb/griffin/SqlHints.java`	Added `ENABLE_PRE_TOUCH_HINT` constant and `hasEnablePreTouchHint()` method; renamed `hasAsof` methods to `hasAsOf` (camelCase)
Code Generation `core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java`	Integrated hint-based pre-touch detection; wires `enablePreTouch` flag from hints into async filter factory constructors
Async Filter Factories `core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterAtom.java`, `core/src/main/java/io/questdb/griffin/engine/table/AsyncFilteredRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java`	Updated constructors to accept `enablePreTouch` boolean parameter, replacing prior runtime pre-touch configuration checks
Filter & Join Factories `core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/LatestByLightRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/LatestByRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/LatestBySubQueryRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/LimitRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/table/CountRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/join/...RecordCursorFactory.java` (11 join factories), `core/src/main/java/io/questdb/griffin/engine/orderby/LimitedSizeSortedLightRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/union/AbstractSetRecordCursorFactory.java`, `core/src/main/java/io/questdb/griffin/engine/window/...RecordCursorFactory.java`	Removed forced disabling of column pre-touch via `executionContext.setColumnPreTouchEnabled(false)` calls
REST API `core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java`	Removed runtime pre-touch override based on query stop condition
Configuration Files `core/src/main/resources/io/questdb/site/conf/server.conf`, `core/src/test/resources/server.conf`, `pkg/ami/marketplace/assets/server.conf`	Removed `cairo.sql.parallel.filter.pretouch.enabled` configuration options
Test Infrastructure `core/src/test/java/io/questdb/test/cutlass/text/SqlExecutionContextStub.java`, `core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java`, `core/src/test/java/io/questdb/test/griffin/engine/table/AsyncFilteredRecordCursorFactoryTest.java`	Removed pre-touch delegation methods from stub; updated test methods to use `ENABLE_PRE_TOUCH` hints
Test Expectations `core/src/test/java/io/questdb/test/PropServerConfigurationTest.java`, `core/src/test/java/io/questdb/test/ServerMainQuerySmokeTest.java`, `core/src/test/java/io/questdb/test/ServerMainTest.java`, `core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java`, `core/src/test/java/io/questdb/test/griffin/...Test.java` (20+ test files)	Updated assertions to reflect new hint-based system; removed `[pre-touch]` annotations from expected plan outputs; added `ENABLE_PRE_TOUCH` hints in query strings where needed

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant SqlCodeGenerator
    participant SqlHints
    participant AsyncFilterFactory
    participant AsyncFilterAtom

    Client->>SqlCodeGenerator: generateCode(queryModel)
    activate SqlCodeGenerator
    
    SqlCodeGenerator->>SqlHints: hasEnablePreTouchHint(queryModel, tableName)
    activate SqlHints
    Note over SqlHints: Check for ENABLE_PRE_TOUCH hint in hints map
    SqlHints-->>SqlCodeGenerator: enablePreTouch: boolean
    deactivate SqlHints
    
    alt enablePreTouch == true
        SqlCodeGenerator->>AsyncFilterFactory: new AsyncFilterFactory(..., enablePreTouch=true)
        AsyncFilterFactory->>AsyncFilterAtom: new AsyncFilterAtom(..., enablePreTouch=true)
        activate AsyncFilterAtom
        Note over AsyncFilterAtom: Store enablePreTouch flag, use for plan rendering
        deactivate AsyncFilterAtom
    else enablePreTouch == false
        SqlCodeGenerator->>AsyncFilterFactory: new AsyncFilterFactory(..., enablePreTouch=false)
        AsyncFilterFactory->>AsyncFilterAtom: new AsyncFilterAtom(..., enablePreTouch=false)
        activate AsyncFilterAtom
        Note over AsyncFilterAtom: Use default behavior (no pre-touch)
        deactivate AsyncFilterAtom
    end
    
    AsyncFilterFactory-->>SqlCodeGenerator: factory instance
    deactivate SqlCodeGenerator
    SqlCodeGenerator-->>Client: cursor with pre-touch (if hint specified)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

This PR spans multiple architectural concerns: configuration API removal and deprecation, introduction of a new SQL hints framework, extensive factory refactoring across filter and join operations, code generation updates, and comprehensive test adjustments. While many changes follow a repetitive pattern (removing pre-touch toggles), the heterogeneity of changes across configuration, code generation, factory construction, and hint integration requires separate reasoning for each area. The removal of public APIs and their replacement with a hint-based system introduces meaningful logic density and integration points that demand careful review.

Possibly related PRs

questdb/questdb#6158: Modifies SqlCodeGenerator for join/filter construction logic alongside this PR's hint-based pre-touch integration.
questdb/questdb#5805: Changes core execution context classes (SqlExecutionContext, SqlExecutionContextImpl) affected by pre-touch method removals in this PR.

Suggested reviewers

bluestreak01
ideoma

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 1.80% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly summarizes the main change by indicating the disabling of column pre-touch in parallel filter operations, using a concise and descriptive phrase. It aligns with conventional commit conventions and provides immediate insight into the performance-focused SQL update.
Description Check	✅ Passed	The description clearly explains the rationale for disabling column pre-touch in parallel filters, outlines the deprecation of the existing configuration property, and specifies how to enable pre-touch via a new SQL hint. It directly corresponds to the actual changes in the pull request and provides relevant context for reviewers.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

puzpuzpuz · 2025-10-16T11:29:17Z

@coderabbitai review

coderabbitai · 2025-10-16T11:29:57Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (4)

core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java (1)
514-530: LGTM! Good test coverage for the new ENABLE_PRE_TOUCH hint.

The test correctly validates the new SQL hint functionality by exercising the parallel filter path with pre-touch enabled. The structure follows existing patterns, and the expected results are consistent with similar filter tests.

Optional improvement: For consistency with other test pairs in this file (e.g., lines 533-543 showing JIT disabled/enabled variants), consider adding a corresponding test with SqlJitMode.JIT_MODE_ENABLED:
@Test
public void testParallelStressSymbolEnablePreTouchJitEnabled() throws Exception {
    // Disable the test on ARM64.
    Assume.assumeTrue(JitUtil.isJitSupported());

    testParallelStressSymbol(
            "select /*+ ENABLE_PRE_TOUCH(x) */ * from x where v > 3326086085493629941L and v < 4326086085493629941L and s ~ 'A' order by v",
            "v\ts\n" +
                    "3393210801760647293\tA\n" +
                    "3433721896286859656\tA\n" +
                    "3619114107112892010\tA\n" +
                    "3669882909701240516\tA\n" +
                    "3820631780839257855\tA\n" +
                    "4039070554630775695\tA\n" +
                    "4290477379978201771\tA\n",
            4,
            1,
            SqlJitMode.JIT_MODE_ENABLED
    );
}
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)

313-313: Avoid wildcard static import to reduce collision risk

This works, but wildcard static imports can introduce accidental name clashes over time. Prefer importing the specific constants actually used.

core/src/test/java/io/questdb/test/griffin/SqlHintsTest.java (1)

102-117: Good coverage for ENABLE_PRE_TOUCH; consider alias/any-table cases

Solid basic checks (present/absent, case-insensitive, clear). To harden:

Add test where table is aliased in SQL (ensure hint resolution works with alias vs base name).

Add test for multiple tables in hint payload (if supported) and mismatched names returning false.

Add test with tableName = null to verify the “any table” behavior (since API accepts @nullable).

core/src/test/java/io/questdb/test/griffin/CompiledFilterTest.java (1)

694-701: Confirm updated assertQueryNoLeakCheck signature

New trailing boolean argument: ensure this matches the latest helper signature and intended behavior (e.g., plan checks, order guarantees, or leak-check bypass). If this is now required, consider a short comment to indicate its purpose for future readers.

Also applies to: 728-734

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93f2484 and 9137898.

📒 Files selected for processing (66)

core/src/main/java/io/questdb/PropServerConfiguration.java (2 hunks)
core/src/main/java/io/questdb/cairo/CairoConfiguration.java (0 hunks)
core/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.java (0 hunks)
core/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.java (0 hunks)
core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java (0 hunks)
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (8 hunks)
core/src/main/java/io/questdb/griffin/SqlExecutionContext.java (0 hunks)
core/src/main/java/io/questdb/griffin/SqlExecutionContextImpl.java (0 hunks)
core/src/main/java/io/questdb/griffin/SqlHints.java (2 hunks)
core/src/main/java/io/questdb/griffin/engine/LimitRecordCursorFactory.java (1 hunks)
core/src/main/java/io/questdb/griffin/engine/groupby/CountRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinLightNoKeyRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashJoinLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinFilteredLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinFilteredRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/LtJoinLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/LtJoinNoKeyRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/LtJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopFullJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopLeftJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopRightJoinRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/join/SpliceJoinLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/orderby/LimitedSizeSortedLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterAtom.java (4 hunks)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilteredRecordCursorFactory.java (2 hunks)
core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java (3 hunks)
core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/table/LatestByLightRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/table/LatestByRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/table/LatestBySubQueryRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/union/AbstractSetRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/window/CachedWindowRecordCursorFactory.java (0 hunks)
core/src/main/java/io/questdb/griffin/engine/window/WindowRecordCursorFactory.java (0 hunks)
core/src/main/resources/io/questdb/site/conf/server.conf (0 hunks)
core/src/test/java/io/questdb/test/PropServerConfigurationTest.java (0 hunks)
core/src/test/java/io/questdb/test/ServerMainQuerySmokeTest.java (1 hunks)
core/src/test/java/io/questdb/test/ServerMainTest.java (0 hunks)
core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/text/SqlExecutionContextStub.java (0 hunks)
core/src/test/java/io/questdb/test/griffin/CompiledFilterTest.java (6 hunks)
core/src/test/java/io/questdb/test/griffin/DeclareTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (85 hunks)
core/src/test/java/io/questdb/test/griffin/GroupByTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/OrderByWithFilterTest.java (3 hunks)
core/src/test/java/io/questdb/test/griffin/ParallelFilterTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/SqlHintsTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/SqlOptimiserTest.java (16 hunks)
core/src/test/java/io/questdb/test/griffin/WhereClauseParserTest.java (2 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/bool/WithinGeohashFunctionFactoryTest.java (3 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/date/IntervalFunctionTest.java (2 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/eq/EqTimestampCursorFunctionFactoryTest.java (2 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/lt/GtTimestampCursorFunctionFactoryTest.java (10 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/lt/LtTimestampCursorFunctionFactoryTest.java (8 hunks)
core/src/test/java/io/questdb/test/griffin/engine/functions/regex/LikeSymbolFunctionFactoryTest.java (7 hunks)
core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByNanoTimestampTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByTest.java (1 hunks)
core/src/test/java/io/questdb/test/griffin/engine/table/AsyncFilteredRecordCursorFactoryTest.java (1 hunks)
core/src/test/resources/server.conf (0 hunks)
pkg/ami/marketplace/assets/server.conf (0 hunks)

💤 Files with no reviewable changes (37)

core/src/main/java/io/questdb/griffin/engine/join/HashJoinLightRecordCursorFactory.java
core/src/main/java/io/questdb/cairo/CairoConfiguration.java
core/src/main/resources/io/questdb/site/conf/server.conf
core/src/main/java/io/questdb/cairo/DefaultCairoConfiguration.java
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopLeftJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinRecordCursorFactory.java
core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java
core/src/main/java/io/questdb/griffin/engine/join/LtJoinNoKeyRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/union/AbstractSetRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/SpliceJoinLightRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/table/LatestByLightRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopRightJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/window/WindowRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinFilteredLightRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/SqlExecutionContext.java
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinLightRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/window/CachedWindowRecordCursorFactory.java
core/src/test/java/io/questdb/test/cutlass/text/SqlExecutionContextStub.java
core/src/main/java/io/questdb/griffin/engine/table/LatestByRecordCursorFactory.java
core/src/main/java/io/questdb/cairo/CairoConfigurationWrapper.java
core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/HashJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinLightNoKeyRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/AsOfJoinLightRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/join/LtJoinLightRecordCursorFactory.java
core/src/test/java/io/questdb/test/PropServerConfigurationTest.java
core/src/main/java/io/questdb/griffin/engine/join/HashOuterJoinFilteredRecordCursorFactory.java
core/src/test/java/io/questdb/test/ServerMainTest.java
core/src/main/java/io/questdb/griffin/engine/join/LtJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/groupby/CountRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/table/LatestBySubQueryRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/SqlExecutionContextImpl.java
core/src/test/resources/server.conf
core/src/main/java/io/questdb/griffin/engine/join/NestedLoopFullJoinRecordCursorFactory.java
core/src/main/java/io/questdb/griffin/engine/orderby/LimitedSizeSortedLightRecordCursorFactory.java
pkg/ami/marketplace/assets/server.conf

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)

GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
GitHub Check: New pull request (Hosted Running tests on windows-other)
GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
GitHub Check: New pull request (Hosted Running tests on windows-cairo)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
GitHub Check: New pull request (Hosted Running tests on windows-griffin)
GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
GitHub Check: New pull request (Hosted Running tests on mac-other)
GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
GitHub Check: New pull request (Hosted Running tests on mac-cairo)
GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
GitHub Check: New pull request (Hosted Running tests on mac-griffin)
GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
GitHub Check: New pull request (Check Changes Check changes)

🔇 Additional comments (43)

core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByNanoTimestampTest.java (1)

6394-6394: Expectation aligns with new default

Dropping the “[pre-touch]” marker matches the new default where filters no longer pre-touch unless hinted. Looks good.

core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByTest.java (1)

6722-6722: Expectation update matches new planner output

Removing the [pre-touch] annotation keeps this plan expectation consistent with the new default (pre-touch disabled unless hinted). Looks good.

core/src/main/java/io/questdb/PropServerConfiguration.java (1)

2666-2667: Deprecation wiring looks good.

Adding CAIRO_SQL_PARALLEL_FILTER_PRETOUCH_ENABLED to the deprecated registry keeps legacy configs emitting advisory warnings without breaking startup. Nice touch.

core/src/test/java/io/questdb/test/griffin/GroupByTest.java (1)

2207-2207: LGTM! Test expectation correctly updated.

The removal of the [pre-touch] annotation from the expected plan output correctly reflects the PR's change to disable column pre-touch by default in parallel filters. The filter condition (seller='sf' or buyer='sf') remains properly validated.

core/src/test/java/io/questdb/test/griffin/engine/functions/date/IntervalFunctionTest.java (1)

378-379: LGTM! Test expectations correctly updated.

The expected query plan output has been updated to remove the [pre-touch] annotation, correctly reflecting the new default behavior where column pre-touch is disabled unless explicitly enabled via SQL hint.

Also applies to: 390-391

core/src/test/java/io/questdb/test/griffin/WhereClauseParserTest.java (2)

3606-3606: Expectation update looks good.

Dropping the [pre-touch] suffix keeps the plan assertion in sync with the new default behaviour. No issues spotted.

3616-3616: OK on the second plan string as well.

This assertion now matches the updated planner output (no implicit pre-touch flag). Looks good.

core/src/test/java/io/questdb/test/griffin/engine/functions/eq/EqTimestampCursorFunctionFactoryTest.java (1)

189-189: LGTM! Test expectations correctly updated.

The removal of [pre-touch] annotations from the EXPLAIN plan outputs correctly reflects the PR's change to disable column pre-touch by default in parallel filters. Pre-touch is now only enabled via explicit SQL hints.

Also applies to: 423-423

core/src/test/java/io/questdb/test/griffin/engine/functions/lt/LtTimestampCursorFunctionFactoryTest.java (1)

471-471: LGTM! Test expectations correctly updated.

The removal of [pre-touch] annotations from the EXPLAIN plan outputs across both thread-safe and state-shared query plan variants correctly reflects the PR's change to disable column pre-touch by default.

Also applies to: 486-486, 501-501, 516-516, 533-533, 548-548, 563-563, 578-578

core/src/test/java/io/questdb/test/griffin/engine/functions/lt/GtTimestampCursorFunctionFactoryTest.java (1)

457-457: LGTM! Test expectations correctly updated.

The removal of [pre-touch] annotations from the EXPLAIN plan outputs across multiple operator variants (> and <=) and execution modes (thread-safe and state-shared) correctly reflects the PR's change to disable column pre-touch by default.

Also applies to: 472-472, 487-487, 502-502, 517-517, 532-532, 548-548, 563-563, 578-578, 593-593

core/src/test/java/io/questdb/test/griffin/engine/functions/regex/LikeSymbolFunctionFactoryTest.java (2)

68-77: LGTM! Test expectation correctly updated.

The removal of [pre-touch] from the expected query plan aligns with the PR objective to disable column pre-touch by default in parallel filters.

485-546: LGTM! All query plan expectations correctly updated.

The systematic removal of [pre-touch] from all LIKE/ILIKE query plan assertions (lines 488, 498, 510, 520, 531, 541) is consistent with disabling pre-touch by default. The test continues to verify correct filter behavior across various pattern types.

core/src/test/java/io/questdb/test/griffin/OrderByWithFilterTest.java (3)

333-333: LGTM! Test expectation updated correctly.

The removal of the [pre-touch] suffix from the filter plan output correctly reflects the new default behavior where pre-touch is disabled unless explicitly enabled via the ENABLE_PRE_TOUCH hint.

373-373: LGTM! Consistent update.

The test expectation is updated consistently with the previous change, correctly reflecting the disabled pre-touch default behavior.

471-471: Test coverage for ENABLE_PRE_TOUCH hint is comprehensive and verified.

The hint functionality is covered extensively:

SqlHintsTest.java: Unit tests for hasEnablePreTouchHint() method

CompiledFilterTest.java: Multiple tests with the hint across various query patterns

ParallelFilterTest.java: Dedicated testParallelStressSymbolEnablePreTouch() test

ExplainPlanTest.java: Plan verification showing the [pre-touch] suffix in output

AsyncFilteredRecordCursorFactoryTest.java, IODispatcherTest.java, ServerMainQuerySmokeTest.java: Additional integration and smoke tests

The change at line 471 correctly removes the [pre-touch] suffix, consistent with lines 333 and 373.

core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java (3)

3838-3855: LGTM! Test correctly reflects pre-touch disabled by default.

The changes are appropriate:

The StringSink optimization (lines 3838, 3845) reduces allocations by reusing a single instance across iterations—good practice.

The expected filter output (line 3852) correctly omits the [pre-touch] suffix, aligning with the PR's goal of disabling pre-touch by default.

3858-3864: LGTM! Extended mode expectations consistent with simple mode.

The expected filter output (line 3861) correctly omits the [pre-touch] suffix, maintaining consistency with the simple mode branch and the PR's objectives.

9644-9650: ENABLE_PRE_TOUCH hint tests verified SqlHintsTest covers parsing and ExplainPlanTest confirms “[pre-touch]” in query plans.

core/src/test/java/io/questdb/test/ServerMainQuerySmokeTest.java (1)

251-251: LGTM! Test correctly uses the new pre-touch hint.

The SQL query now includes the ENABLE_PRE_TOUCH(tab) hint, and the expected plan output correctly shows [pre-touch] in the filter line. This properly demonstrates the new hint-based pre-touch mechanism.

core/src/main/java/io/questdb/griffin/engine/table/AsyncFilteredRecordCursorFactory.java (1)

79-80: LGTM! Parameter properly added and propagated.

The enablePreTouch parameter is correctly added to the constructor signature and properly propagated to the AsyncFilterAtom. This enables hint-based control of pre-touch behavior at the factory level.

Also applies to: 98-99

core/src/test/java/io/questdb/test/griffin/engine/table/AsyncFilteredRecordCursorFactoryTest.java (1)

653-659: LGTM! Test correctly migrated to hint-based pre-touch.

The test is properly updated to use the ENABLE_PRE_TOUCH(x) SQL hint instead of configuration-based toggles. The renamed test method accurately reflects that it's testing enabled pre-touch behavior.

core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterAtom.java (3)

55-75: LGTM! Pre-touch control properly migrated to constructor parameter.

The preTouchEnabled field is correctly introduced as an immutable constructor parameter, replacing the previous configuration-based approach. The field is thread-safe since it's set once in the constructor and never modified.

135-135: LGTM! Direct field access simplifies pre-touch check.

Using the preTouchEnabled field directly is cleaner and more efficient than the previous configuration-based checks.

223-225: LGTM! Plan output correctly reflects pre-touch state.

The plan output correctly displays the [pre-touch] annotation when pre-touch is enabled.

core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java (1)

91-92: LGTM! JIT version consistently updated with pre-touch parameter.

The enablePreTouch parameter is properly added to both the factory and atom constructors, maintaining consistency with the non-JIT versions. The parameter is correctly propagated through the constructor chain.

Also applies to: 121-122, 443-446

core/src/test/java/io/questdb/test/griffin/DeclareTest.java (1)

901-901: LGTM! Expected plan correctly updated for default pre-touch behavior.

The removal of [pre-touch] from the expected plan output correctly reflects that pre-touch is no longer enabled by default. Pre-touch now requires an explicit SQL hint.

core/src/test/java/io/questdb/test/griffin/engine/functions/bool/WithinGeohashFunctionFactoryTest.java (1)

118-118: LGTM! Test expectations consistently updated across multiple test methods.

The removal of [pre-touch] from expected plan outputs across testBindVariables, testConstVarFilter, and testVarConstFilter correctly reflects the new default behavior where pre-touch is disabled unless explicitly requested via hint.

Also applies to: 159-159, 233-233

core/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java (1)

144-144: LGTM! Test expectation correctly updated for default behavior.

The removal of [pre-touch] from the expected plan output in testAllVariables correctly reflects that pre-touch is no longer enabled by default.

core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java (1)

4603-4625: Pre-touch hint coverage looks solid.

Great to see the JSON endpoint exercised with the ENABLE_PRE_TOUCH hint; the request and expected payload align with the new per-query toggle semantics, so the test should catch any regressions here.
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (4)
2481-2481: Hint-driven pre‑touch propagation into async filters looks good

Computing enablePreTouch via SqlHints and threading it through Async(Jit)FilteredRecordCursorFactory is correct.

Please confirm:

hasEnablePreTouchHint returns false when the hint is absent and gracefully handles a null/unknown table name.

Deprecated server-side flag is indeed ignored in these code paths, keeping behavior strictly hint-driven.

Also applies to: 2523-2525, 2566-2568

2753-2755: AsOf join hints usage LGTM

Using hasAsOfLinearSearchHint and hasAsOfIndexSearchHint is consistent with the AsOf join branch.

3211-3213: Pre‑touch hint lookup may receive null alias

masterAlias is set to null earlier in several branches; passing it into hasEnablePreTouchHint may disable pre‑touch unintentionally if null isn’t handled.

Consider a safe fallback:
- SqlHints.hasEnablePreTouchHint(model, masterAlias)
+ SqlHints.hasEnablePreTouchHint(model, masterAlias != null ? masterAlias : model.getName())
Or, if hints are query‑level (not table‑scoped) in this context, pass null intentionally and ensure the method treats null as “any table”.

Also applies to: 3263-3265

3029-3030: Remove non-existent method suggestion
SqlHints.java does not define hasLtLinearSearchHint; if a dedicated LT linear-search hint is required, add hasLtLinearSearchHint(...) to SqlHints, otherwise rename ltLinearSearch to something like linearSearchHint to clarify reuse of the AsOf hint.

Likely an incorrect or invalid review comment.
core/src/test/java/io/questdb/test/griffin/SqlHintsTest.java (2)

62-77: Renamed AsOf index-search hint checks look correct

Method rename usage aligns with API and keeps case/order coverage.

84-100: Renamed AsOf linear-search hint checks look correct

Usage updated consistently; case/order paths preserved.

core/src/test/java/io/questdb/test/griffin/CompiledFilterTest.java (4)

56-56: Verify threshold property remains valid

Setting CAIRO_SQL_PARALLEL_FILTER_PRETOUCH_THRESHOLD globally for this class is fine, but please confirm this key is still honored with hint-driven pre-touch and not deprecated in this PR.

243-250: Inline hint usage is appropriate

Enabling pre-touch via /*+ ENABLE_PRE_TOUCH(t1) */ matches the new model and keeps tests local to the query.

640-663: Helper encapsulation is clean; good reuse

The local setup (DDL + insert) plus param-driven query keeps scenarios focused. Looks good.

666-675: Conditional pre-touch hint injection reads well

The ternary-based insertion of ENABLE_PRE_TOUCH for t1 is clear and consistent across helpers.

Also applies to: 678-691, 739-748

core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (1)

615-639: Nice coverage for the new hint-based flow

The extra assertion ensures we now have explicit coverage for both the default (no pre-touch) path and the hint-enabled one—great way to lock in the new behaviour.

core/src/main/java/io/questdb/griffin/SqlHints.java (3)

41-41: LGTM!

The new constant follows the established naming convention and aligns with the PR objective of introducing per-query pre-touch control via SQL hints.

48-62: All method renames have been properly updated across the codebase.

Verification confirms that the old method names hasAsofIndexSearchHint and hasAsofLinearSearchHint do not exist anywhere in the codebase, and the new method names hasAsOfIndexSearchHint and hasAsOfLinearSearchHint are correctly defined at lines 48 and 56 in SqlHints.java. All call sites have been successfully updated to use the renamed methods.

88-96: Null safety confirmed in containsWordIgnoreCase. Returns false when either seq or term is null (covered by existing tests), so nullable tableName is handled safely.

glasstiger · 2025-10-16T12:19:09Z

[PR Coverage check]

😍 pass : 15 / 19 (78.95%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/SqlCodeGenerator.java	6	10	60.00%
🔵	io/questdb/PropServerConfiguration.java	2	2	100.00%
🔵	io/questdb/griffin/engine/table/AsyncFilterAtom.java	3	3	100.00%
🔵	io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java	1	1	100.00%
🔵	io/questdb/griffin/SqlHints.java	3	3	100.00%

perf(sql): disable column pre-touch in parallel filter

9137898

puzpuzpuz self-assigned this Oct 16, 2025

puzpuzpuz added SQL Issues or changes relating to SQL execution Performance Performance improvements regression labels Oct 16, 2025

coderabbitai bot reviewed Oct 16, 2025

View reviewed changes

Fix tests

b83abf7

bluestreak01 approved these changes Oct 16, 2025

View reviewed changes

bluestreak01 merged commit 67abe2f into master Oct 16, 2025
35 checks passed

bluestreak01 deleted the puzpuzpuz_disable_pre_touch branch October 16, 2025 18:15

coderabbitai bot mentioned this pull request Nov 28, 2025

perf(sql): improve query scalability on large multicore machines #6459

Merged

coderabbitai bot mentioned this pull request Jan 26, 2026

feat(core): optimize parquet partition read with late materialization, zero-copy page reading, and use raw array encoding #6675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(sql): disable column pre-touch in parallel filter#6280

perf(sql): disable column pre-touch in parallel filter#6280
bluestreak01 merged 2 commits intomasterfrom
puzpuzpuz_disable_pre_touch

puzpuzpuz commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

Review skipped

Uh oh!

puzpuzpuz commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

glasstiger commented Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

puzpuzpuz commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

puzpuzpuz commented Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 16, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

glasstiger commented Oct 16, 2025

[PR Coverage check]

file detail

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Oct 16, 2025 •

edited

Loading