feat(http): REST API for SQL validation by bluestreak01 · Pull Request #6383 · questdb/questdb

bluestreak01 · 2025-11-11T15:49:49Z

Introduces new APIs:

/api/v1/sql/validate
/api/v1/sql/execute

This is a non-breaking change. The existing /exec api is still supported although it becomes deprecated with this PR

Tandem: https://github.com/questdb/questdb-enterprise/pull/782

Implementation

The new processor was a copy of json processor, sans sending data to the client. I added fuzz tests for fragmentation to find out this copy is badly broken. This sent us on a bug hunt in all other processors and to extend fuzz tests. The net result are bugs fixed in:

json processor
csv/parquet export processor
http client

More test infrastructure was added to enable robust fuzz testing of http APIs

coderabbitai · 2025-11-11T15:49:55Z

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 pull request removes the file-watching subsystem (KqueueFileWatcher, FileWatcher, LinuxFileWatcher, WindowsAccessor implementations) and replaces ObjList with ObjHashSet throughout HTTP context path collections. It introduces SqlValidationProcessor, a new HTTP processor for SQL validation, and updates HTTP server endpoint wiring to support SQL validation. Related tests and module exports are updated accordingly.

Changes

Cohort / File(s)	Summary
File Watcher Removal `core/src/main/java/io/questdb/KqueueFileWatcher.java`, `core/src/main/java/io/questdb/std/filewatch/FileWatcher.java`, `core/src/main/java/io/questdb/std/filewatch/FileWatcherFactory.java`, `core/src/main/java/io/questdb/std/filewatch/FileWatcherWindows.java`, `core/src/main/java/io/questdb/std/filewatch/LinuxFileWatcher.java`, `core/src/main/java/io/questdb/std/filewatch/LinuxAccessor.java`, `core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacade.java`, `core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacadeImpl.java`, `core/src/main/java/io/questdb/std/filewatch/WindowsAccessor.java`, `core/src/main/java/io/questdb/std/filewatch/WindowsAccessorImpl.java`	Deleted entire file-watching abstraction including platform-specific implementations (kqueue, Linux inotify, Windows directory watching), factory, and related interfaces.
Server Configuration Updates `core/src/main/java/io/questdb/ServerMain.java`, `core/src/main/java/io/questdb/PropServerConfiguration.java`	Removed FileWatcher integration from ServerMain. Updated PropServerConfiguration to use ObjHashSet for HTTP context paths and add SQL validation path accessors.
Collection Type Migration (ObjList → ObjHashSet) `core/src/main/java/io/questdb/cutlass/http/HttpFullFatServerConfiguration.java`, `core/src/main/java/io/questdb/cutlass/http/HttpRequestHandlerFactory.java`, `core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java`, `core/src/main/java/io/questdb/cutlass/http/HttpServerConfigurationWrapper.java`, `core/src/main/java/io/questdb/cutlass/http/processors/StaticContentProcessorFactory.java`	Replaced ObjList with ObjHashSet for context path collections. Added public constants for context paths in HttpFullFatServerConfiguration and updated method return types across configuration classes.
SQL Validation Processor `core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java`, `core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java`	Introduced new SqlValidationProcessor for HTTP-driven SQL validation with query compilation, execution, and JSON response formatting. Added SqlValidationProcessorState to manage response state machine and metadata collection.
HTTP Server Integration `core/src/main/java/io/questdb/cutlass/http/HttpServer.java`, `core/src/main/java/io/questdb/cutlass/Services.java`	Updated HttpServer.addDefaultEndpoints to wire SqlValidationProcessor; renamed ilpWriteProcessorBuilderV2 to ilpV2WriteProcessorBuilder. Changed URL iteration to use ObjHashSet with get(j) instead of getQuick(j). Updated Services to instantiate and wire sqlValidationProcessorBuilder.
Module Exports `core/src/main/java/module-info.java`, `core/src/test/java/module-info.java`	Removed public export of io.questdb.std.filewatch package from core module; removed export of io.questdb.test.std.filewatch from test module.
Test File Updates `core/src/test/java/io/questdb/test/PropServerConfigurationTest.java`, `core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java`, `core/src/test/java/io/questdb/test/cutlass/http/HttpMinTestBuilder.java`, `core/src/test/java/io/questdb/test/cutlass/http/HttpQueryTestBuilder.java`, `core/src/test/java/io/questdb/test/cutlass/http/HttpServerConfigurationBuilder.java`, `core/src/test/java/io/questdb/test/cutlass/http/HttpServerTest.java`, `core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java`, `core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderMockServerTest.java`	Updated test implementations to use ObjHashSet for getUrls() return types. Added fragmentation chunk size configuration to test builders. Updated assertions to use ObjHashSet instead of ObjList.
SQL Validation Tests `core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java`	Added new test class with three test methods validating SQL validation endpoint: testValidationAllColumnTypes, testValidationOk, testValidationSyntaxError.
File Watcher Tests Removal `core/src/test/java/io/questdb/test/std/filewatch/FileWatcherTest.java`, `core/src/test/java/io/questdb/test/std/filewatch/LinuxFileWatcherTest.java`	Deleted FileWatcherTest and LinuxFileWatcherTest, removing all file-watcher unit tests and infrastructure.
Utility Test Updates `utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java`	Added SqlValidationProcessor builder instantiation and wiring to HttpServer.addDefaultEndpoints call.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HttpServer
    participant SqlValidationProcessor
    participant SqlCompiler
    participant Cairo

    Client->>HttpServer: POST /api/v1/sql/validate<br/>(query)
    HttpServer->>SqlValidationProcessor: onRequestComplete()
    SqlValidationProcessor->>SqlCompiler: compile(query)
    
    alt SELECT query
        SqlCompiler-->>SqlValidationProcessor: CompiledQuery (SELECT)
        SqlValidationProcessor->>Cairo: executeNewSelect()
        Cairo-->>SqlValidationProcessor: RecordCursor with metadata
        SqlValidationProcessor->>SqlValidationProcessor: serialize columns to JSON
        SqlValidationProcessor-->>Client: 200 OK<br/>{query, columns[]}
    else DDL/DML query
        SqlCompiler-->>SqlValidationProcessor: CompiledQuery (INSERT/CREATE/etc)
        SqlValidationProcessor->>SqlValidationProcessor: buildConfirmation(type)
        SqlValidationProcessor-->>Client: 200 OK<br/>{type: "INSERT"/<br/>"CREATE"/etc}
    else Syntax error
        SqlCompiler-->>SqlValidationProcessor: SqlException
        SqlValidationProcessor->>SqlValidationProcessor: formatErrorResponse()
        SqlValidationProcessor-->>Client: 400 Bad Request<br/>{error, position}
    end

sequenceDiagram
    participant OldFlow as File Watcher Flow<br/>(Removed)
    participant NewFlow as Collection Type<br/>Migration
    participant Processor as SQL Validation<br/>(New)

    OldFlow->>OldFlow: FileWatcher monitors config files
    OldFlow->>OldFlow: Triggers on changes via<br/>platform-specific watchers<br/>(kqueue/inotify/Windows)
    
    NewFlow->>NewFlow: HTTP context paths<br/>now use ObjHashSet<br/>(instead of ObjList)
    NewFlow->>NewFlow: Improves lookup performance<br/>for URL routing
    
    Processor->>Processor: New HTTP endpoint<br/>validates SQL queries
    Processor->>Processor: Returns column metadata<br/>and query confirmation

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45–60 minutes

Areas requiring extra attention:

SqlValidationProcessor.java — New processor with complex state machine, error handling across multiple exception types (SqlException, CairoException, DataUnavailableException, etc.), and JSON serialization logic. Verify correct handling of query types, metadata collection, and partial response writes.
SqlValidationProcessorState.java — State management class with multiple response phases and column metadata validation. Ensure state transitions are correct and resource cleanup is thorough.
HttpServer.java — Critical changes to addDefaultEndpoints signature and URL iteration logic (ObjHashSet with get(j) vs. ObjList with getQuick(j)). Verify all endpoint bindings are wired correctly and URL routing remains functional.
Collection type migration consistency — Verify that all callers of context-path getters properly handle ObjHashSet instead of ObjList, particularly around iteration and access patterns.
PropServerConfigurationTest.java — Test logic refactored to use ObjHashSet; verify fuzz test structure and expected value calculations match new assertion model.

Suggested labels

Enhancement, SQL, Core

Suggested reviewers

puzpuzpuz

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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 PR title 'feat(http): REST API for SQL validation' accurately reflects the main change: introducing new HTTP REST APIs for SQL validation (/api/v1/sql/validate and /api/v1/sql/execute).
Description check	✅ Passed	The PR description accurately describes the changes: introducing new SQL validation/execute APIs and noting this is non-breaking with backward compatibility for /exec.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.}

bluestreak01 · 2025-11-16T02:02:43Z

@CodeRabbit review

coderabbitai · 2025-11-16T02:02:48Z

✅ 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: 5

🧹 Nitpick comments (12)

core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java (2)
2326-2330: Avoid double‑brace initialization; build the set explicitly.

Double‑brace creates an anonymous subclass and an extra synthetic reference. Prefer constructing, adding, returning; pre‑size to 1.

Apply:
-                    public ObjHashSet<String> getUrls() {
-                        return new ObjHashSet<>() {{
-                            add("/upload");
-                        }};
-                    }
+                    public ObjHashSet<String> getUrls() {
+                        ObjHashSet<String> urls = new ObjHashSet<>(1);
+                        urls.add("/upload");
+                        return urls;
+                    }
8351-8355: Same here: replace double‑brace with explicit construction.

Keeps allocations minimal and avoids anonymous subclassing.

Apply:
-        public ObjHashSet<String> getUrls() {
-            return new ObjHashSet<>() {{
-                add("/query");
-            }};
-        }
+        public ObjHashSet<String> getUrls() {
+            ObjHashSet<String> urls = new ObjHashSet<>(1);
+            urls.add("/query");
+            return urls;
+        }
core/src/test/java/io/questdb/test/cutlass/http/HttpServerTest.java (1)
171-175: Avoid double‑brace initialization in tests.

Use a plain local, avoids synthetic class and is clearer.
-                public ObjHashSet<String> getUrls() {
-                    return new ObjHashSet<>() {{
-                        add("/service");
-                    }};
-                }
+                public ObjHashSet<String> getUrls() {
+                    ObjHashSet<String> urls = new ObjHashSet<>();
+                    urls.add("/service");
+                    return urls;
+                }
core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java (1)
34-38: Minor: avoid double‑brace init in default method.
-    default ObjHashSet<String> getContextPathMetrics() {
-        return new ObjHashSet<>() {{
-            add("/metrics");
-        }};
-    }
+    default ObjHashSet<String> getContextPathMetrics() {
+        ObjHashSet<String> s = new ObjHashSet<>();
+        s.add("/metrics");
+        return s;
+    }
core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java (2)
184-188: Prefer simple initialization over double‑brace.
-                                    public ObjHashSet<String> getUrls() {
-                                        return new ObjHashSet<>() {{
-                                            add(ILP_TEST_PATH);
-                                        }};
-                                    }
+                                    public ObjHashSet<String> getUrls() {
+                                        ObjHashSet<String> urls = new ObjHashSet<>();
+                                        urls.add(ILP_TEST_PATH);
+                                        return urls;
+                                    }
328-332: Same here: avoid double‑brace initialization.
-                                    public ObjHashSet<String> getUrls() {
-                                        return new ObjHashSet<>(){{
-                                            add(EXEC_TEST_URI);
-                                        }};
-                                    }
+                                    public ObjHashSet<String> getUrls() {
+                                        ObjHashSet<String> urls = new ObjHashSet<>();
+                                        urls.add(EXEC_TEST_URI);
+                                        return urls;
+                                    }
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderMockServerTest.java (2)

682-706: Expanded testWithMock overloads keep call sites simple and resource handling correct

The new overloads that accept a senderBuilderFactory and optional verifyBeforeClose flag centralize setup logic while preserving the previous behavior. The inner implementation still uses try-with-resources for WorkerPool and HttpServer, so lifetimes and leak checks remain correct.

If you touch this again, consider documenting the verifyBeforeClose flag briefly (e.g., Javadoc) so future readers know when to prefer one verification timing over the other.

Also applies to: 709-712

715-719: getUrls() migration to ObjHashSet<String> matches updated HTTP API

Switching the mock HttpRequestHandlerFactory implementations to return ObjHashSet<String> and explicitly adding /write and /settings aligns the test scaffolding with the production HttpRequestHandlerFactory contract and with the new context-path set type.

Double‑brace initialization is fine in tests, but a small helper like urls("/write") / urls("/settings") would avoid anonymous subclasses and be slightly clearer.

Also applies to: 729-733

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

1735-1781: Fuzz test for web-console-derived context paths aligns with ObjHashSet usage but could compare sets more directly

The updated fuzz test:

Models each configurable path as a FuzzItem carrying the property key, pinned value, default web‑console suffix set (CONTEXT_PATH_*), and a getter returning the corresponding ObjHashSet<String>.

Randomly pins a subset of paths, then for each item builds an expected ObjHashSet of pinned values plus webConsolePath + suffix for each default suffix, and compares expected.toString() against the actual set’s toString().

Verifies ILP context paths remain equal to HttpFullFatServerConfiguration.CONTEXT_PATH_ILP.toString() and that redirects match the new web-console path.

The logic is sound and gives good coverage of path recomputation.

Instead of relying on ObjHashSet’s toString() ordering, you might make the test resilient to insertion-order changes by:

comparing the sets directly (if ObjHashSet.equals is defined appropriately), or

asserting that both expected and actual contain the same elements (e.g., by iterating and checking membership) without depending on iteration order.
This keeps the test focused on membership semantics rather than internal ordering.

Also applies to: 1797-1819, 2095-2100

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java (1)

223-255: Reuse prepareExecutionContext in resumeSend

Once you introduce a helper to bind sqlExecutionContext to the connection (as suggested earlier), resumeSend() can call it instead of duplicating the .with(...) logic. This keeps the behavior consistent between initial execution and resume paths and reduces the chance of divergence in future edits.
core/src/main/java/io/questdb/cutlass/http/HttpFullFatServerConfiguration.java (2)
34-59: Recommend immutable collections and avoiding double-brace initialization.

The constants use double-brace initialization, which creates anonymous inner classes and is considered an anti-pattern. Additionally, these ObjHashSet instances are mutable and publicly accessible, allowing callers to modify them and potentially causing thread-safety issues or unexpected behavior.

Consider refactoring to use an immutable collection pattern or factory methods that return unmodifiable sets.

Example refactor using factory methods:
-    ObjHashSet<String> CONTEXT_PATH_EXEC = new ObjHashSet<>() {{
-        add("/exec");
-        add("/api/v1/sql/execute");
-    }};
+    ObjHashSet<String> CONTEXT_PATH_EXEC = createImmutableSet("/exec", "/api/v1/sql/execute");
And add a static helper method (if ObjHashSet supports unmodifiable views):
static ObjHashSet<String> createImmutableSet(String... paths) {
    ObjHashSet<String> set = new ObjHashSet<>();
    for (String path : paths) {
        set.add(path);
    }
    // Return unmodifiable view if available
    return set;
}
64-68: Recommend pre-defining constants instead of creating instances on every call.

The methods getContextPathDefault() and getContextPathILPPing() create new ObjHashSet instances on every invocation using double-brace initialization. This is inefficient and inconsistent with other accessor methods that return pre-defined constants.

Consider defining these as constants like the others:
+    ObjHashSet<String> CONTEXT_PATH_DEFAULT = new ObjHashSet<>() {{
+        add(DEFAULT_PROCESSOR_URL);
+    }};
+    ObjHashSet<String> CONTEXT_PATH_ILP_PING = new ObjHashSet<>() {{
+        add("/ping");
+    }};
+
     default ObjHashSet<String> getContextPathDefault() {
-        return new ObjHashSet<>() {{
-            add(DEFAULT_PROCESSOR_URL);
-        }};
+        return CONTEXT_PATH_DEFAULT;
     }
 
     default ObjHashSet<String> getContextPathILPPing() {
-        return new ObjHashSet<>() {{
-            add("/ping");
-        }};
+        return CONTEXT_PATH_ILP_PING;
     }
Also applies to: 82-86

📜 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 c6ee66e and 647edef.

📒 Files selected for processing (35)

core/src/main/java/io/questdb/KqueueFileWatcher.java (0 hunks)
core/src/main/java/io/questdb/PropServerConfiguration.java (6 hunks)
core/src/main/java/io/questdb/ServerMain.java (0 hunks)
core/src/main/java/io/questdb/cutlass/Services.java (6 hunks)
core/src/main/java/io/questdb/cutlass/http/HttpFullFatServerConfiguration.java (1 hunks)
core/src/main/java/io/questdb/cutlass/http/HttpRequestHandlerFactory.java (1 hunks)
core/src/main/java/io/questdb/cutlass/http/HttpServer.java (12 hunks)
core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java (1 hunks)
core/src/main/java/io/questdb/cutlass/http/HttpServerConfigurationWrapper.java (2 hunks)
core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java (1 hunks)
core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java (1 hunks)
core/src/main/java/io/questdb/cutlass/http/processors/StaticContentProcessorFactory.java (2 hunks)
core/src/main/java/io/questdb/std/filewatch/FileWatcher.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/FileWatcherFactory.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/FileWatcherWindows.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/LinuxAccessor.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacade.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacadeImpl.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/LinuxFileWatcher.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/WindowsAccessor.java (0 hunks)
core/src/main/java/io/questdb/std/filewatch/WindowsAccessorImpl.java (0 hunks)
core/src/main/java/module-info.java (0 hunks)
core/src/test/java/io/questdb/test/PropServerConfigurationTest.java (5 hunks)
core/src/test/java/io/questdb/test/cutlass/http/HttpConnectionCountTest.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/http/HttpMinTestBuilder.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/http/HttpQueryTestBuilder.java (11 hunks)
core/src/test/java/io/questdb/test/cutlass/http/HttpServerConfigurationBuilder.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/http/HttpServerTest.java (2 hunks)
core/src/test/java/io/questdb/test/cutlass/http/IODispatcherTest.java (3 hunks)
core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java (1 hunks)
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderMockServerTest.java (6 hunks)
core/src/test/java/io/questdb/test/std/filewatch/FileWatcherTest.java (0 hunks)
core/src/test/java/io/questdb/test/std/filewatch/LinuxFileWatcherTest.java (0 hunks)
core/src/test/java/module-info.java (0 hunks)
utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java (2 hunks)

💤 Files with no reviewable changes (15)

core/src/main/java/io/questdb/std/filewatch/FileWatcher.java
core/src/main/java/module-info.java
core/src/main/java/io/questdb/std/filewatch/LinuxFileWatcher.java
core/src/main/java/io/questdb/std/filewatch/FileWatcherFactory.java
core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacadeImpl.java
core/src/main/java/io/questdb/KqueueFileWatcher.java
core/src/main/java/io/questdb/ServerMain.java
core/src/test/java/io/questdb/test/std/filewatch/FileWatcherTest.java
core/src/main/java/io/questdb/std/filewatch/LinuxAccessor.java
core/src/main/java/io/questdb/std/filewatch/LinuxAccessorFacade.java
core/src/test/java/io/questdb/test/std/filewatch/LinuxFileWatcherTest.java
core/src/main/java/io/questdb/std/filewatch/WindowsAccessorImpl.java
core/src/main/java/io/questdb/std/filewatch/WindowsAccessor.java
core/src/test/java/module-info.java
core/src/main/java/io/questdb/std/filewatch/FileWatcherWindows.java

⏰ 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). (35)

GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
GitHub Check: New pull request (Hosted Running tests on windows-other-2)
GitHub Check: New pull request (Hosted Running tests on windows-other-1)
GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
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-sub)
GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
GitHub Check: New pull request (Hosted Running tests on mac-other)
GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
GitHub Check: New pull request (Hosted Running tests on mac-cairo)
GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
GitHub Check: New pull request (Hosted Running tests on mac-griffin)
GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
GitHub Check: New pull request (Check Changes Check changes)
GitHub Check: build
GitHub Check: build

🔇 Additional comments (41)

core/src/test/java/io/questdb/test/cutlass/http/HttpServerConfigurationBuilder.java (3)

54-55: LGTM: Fragmentation configuration fields added correctly.

The fields follow the existing pattern and sensible defaults (Integer.MAX_VALUE effectively disables fragmentation unless explicitly configured).

162-170: LGTM: Configuration getters implemented correctly.

The overridden methods correctly expose the fragmentation chunk sizes to the HTTP context configuration.

301-309: LGTM: Builder methods follow the fluent API pattern correctly.

The methods are consistent with other builder methods in the class and correctly set the fragmentation chunk sizes.

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

43-109: LGTM: Test implementations updated to match interface change.

Both anonymous HttpRequestHandlerFactory implementations correctly return ObjHashSet<String> from getUrls(), aligning with the interface modification.

core/src/main/java/io/questdb/cutlass/http/processors/StaticContentProcessorFactory.java (1)

30-42: LGTM: Migration from ObjList to ObjHashSet for URL collections.

Verification confirms all implementations of HttpRequestHandlerFactory.getUrls() correctly return ObjHashSet<String> across both test and production code (approximately 29 implementations). The change aligns with the updated interface and is semantically appropriate for URL path collections.

core/src/main/java/io/questdb/cutlass/http/HttpRequestHandlerFactory.java (1)

27-30: LGTM: Interface change improves semantics for URL collections.

Changing from ObjList<String> to ObjHashSet<String> is appropriate for URL collections—it enforces uniqueness and provides O(1) lookup performance.

Verification confirms:

ObjHashSet supports indexed access via get(int index), delegating to its internal ObjList

The single usage in HttpServer.java:293-296 uses indexed iteration but does not depend on ordering

All implementations (TestJsonQueryProcessorFactory, StaticContentProcessorFactory) already return ObjHashSet<String>

No order-dependent patterns found in the codebase

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

112-112: Import update looks good.

Adding ObjHashSet aligns with the interface change to HttpRequestHandlerFactory.getUrls(). No issues.

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

45-45: Import switch to ObjHashSet looks good.

core/src/main/java/io/questdb/cutlass/http/HttpServerConfiguration.java (2)

30-30: Import change to ObjHashSet is correct.

40-44: Wildcard interpretation is incorrect; the code is correct.

The "*" is not a wildcard pattern but a special sentinel value (HttpFullFatServerConfiguration.DEFAULT_PROCESSOR_URL) that designates a fallback/default handler. The HTTP routing in HttpServer.bind() explicitly checks if (DEFAULT_PROCESSOR_URL.equals(url)) and assigns it to selector.defaultRequestProcessor (line 299-300). Request matching first attempts exact URL lookup in requestHandlerMap, then falls back to the default processor if no match is found (line 401).

When metrics are disabled, getContextPathStatus() intentionally returns "*" to indicate the HealthCheckProcessor should act as a fallback handler rather than registering a specific /status path. This design is sound and does not create shadowing issues—the framework properly handles it.

Likely an incorrect or invalid review comment.

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

50-50: Import of ObjHashSet acknowledged.

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

85-85: Import to ObjHashSet is correct.

226-235: New context path sets (execute/validate/etc.) look good.

1073-1076: Default additions include both legacy and new SQL endpoints — OK.

2399-2403: getUrls target type to ObjHashSet aligns with API change.

4648-4655: HTTP server config getters now expose ObjHashSet for paths; wiring for exec + SQL validation is consistent.

Also applies to: 4658-4690

utils/src/test/java/io/questdb/cliutil/Table2IlpTest.java (1)

38-38: SqlValidationProcessor wiring in test helper matches production Services.createHttpServer

The new sqlValidationProcessorBuilder uses the same JSON query configuration and worker-count pattern as JsonQueryProcessor, and HttpServer.addDefaultEndpoints is called with the updated signature including ilpV2WriteProcessorBuilder and sqlValidationProcessorBuilder. This keeps the Table2Ilp CLI test HTTP server in sync with the main Services wiring.

Also applies to: 505-519

core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderMockServerTest.java (2)

109-117: Using text blocks for multi-line expected HTTP bodies improves readability

Replacing concatenated strings with text blocks for multi-line payload expectations (testAutoFlushRows, testJsonError, testTwoLines) keeps the intent clear while preserving the line structure needed for protocol correctness.

Also applies to: 333-337, 609-612

149-195: New tests robustly cover /settings failure handling (single and multi-server)

testBadSettings and testBadSettingsManyServers exercise the error path when the /settings endpoint fails, including the multi-server rotation scenario. The use of MockErrorSettingsProcessor and explicit LineSenderException assertions gives good coverage of the new settings-negotiation behavior.

core/src/main/java/io/questdb/cutlass/Services.java (2)

110-115: HTTP server now cleanly wires in SQL validation alongside JSON and ILP

The introduction of sqlValidationProcessorBuilder and passing it into HttpServer.addDefaultEndpoints mirrors the existing JsonQueryProcessor/LineHttpProcessorImpl pattern and cleanly exposes the new validation endpoint without altering existing behavior.

Also applies to: 121-129

216-218: createMinHttpServer updated to use ObjHashSet<String> for context paths

Returning ObjHashSet<String> from the health and metrics HttpRequestHandlerFactory.getUrls() implementations, delegating to configuration.getContextPathStatus() and getContextPathMetrics(), keeps the minimal HTTP server consistent with the new context-path collection type.

Also applies to: 236-238

core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java (3)

39-53: Static setup/teardown for TestHttpClient looks correct; configuration reassignment merits a quick check

setUpStatic/tearDownStatic correctly free and recreate testHttpClient around the shared test lifecycle, avoiding leaks between runs. You also reassign configuration = new DefaultCairoConfiguration(root) after AbstractCairoTest.setUpStatic(). If AbstractCairoTest’s engine was created from a different configuration instance, just confirm that no code assumes engine.getConfiguration() == configuration; otherwise, this override is fine.

55-122: testValidationAllColumnTypes gives strong coverage of validation metadata

This test creates a table with a wide variety of column types (including arrays and multiple GEOHASH widths) and asserts that /api/v1/sql/validate returns the expected columns array and timestamp index for select * from xyz limit 10. That’s a good end‑to‑end check of the new endpoint’s schema introspection.

Please double‑check that the timestamp value 13 is defined as “zero‑based index of the designated timestamp column” in the SqlValidationProcessor contract; if this semantics ever changes (e.g., to a column name), this test will need adjustment.

124-160: OK and syntax-error validation tests pin down the JSON contract

testValidationOk validates a simple aggregation query, and testValidationSyntaxError asserts that invalid SQL returns a payload with query, error, and position. The hard‑coded position makes this sensitive to parser behavior, but it’s valuable to keep the error-reporting contract stable.

If the SQL parser’s error positioning logic changes frequently, consider asserting that position is non‑negative and that the error message contains "Invalid column: b" rather than pinning the exact numeric offset.

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

1692-1716: Web-console path dependency test correctly includes new SQL execute endpoint

testWebConsolePathChangeUpdatesDefaultDependencies now expects:

warnings under /new-path/warnings,

exec under both /new-path/exec and /new-path/api/v1/sql/execute,

export/import/table-status/settings under the new web-console prefix,
while ILP paths remain unchanged. This is a clear, direct regression test for the new execute endpoint and for context-path rewiring.

core/src/main/java/io/questdb/cutlass/http/HttpServer.java (4)

54-97: Constructor cache initialization & configuration pattern match look good

The switch to pattern matching on HttpFullFatServerConfiguration and keeping the min-server path on the no-op cache is clean and preserves behavior; the added ObjHashSet import is consistent with the rest of the changes in this PR.

130-247: SQL validation endpoint wiring is consistent with existing endpoints

Extending addDefaultEndpoints with sqlValidationProcessorBuilder and binding it via getContextPathSqlValidation() mirrors the existing export/import/tableStatus wiring. Keeping /exec bound through getContextPathExec() ensures backward compatibility while adding the new /api/v1/sql/validate path via configuration.

292-314: Migration to ObjHashSet<String> in bind is safe

Iterating ObjHashSet<String> via size() and get(j) works with its dense storage model, and handler registration logic is unchanged apart from collection type. Since routing is by exact URL match and not order, the move from ObjList to ObjHashSet should not affect behavior.

342-361: handleClientOperation refactor is formatting-only

The extracted helper keeps the same control flow and exception-to-IOOperation mapping as before; no behavioral changes introduced.

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java (1)

59-395: State machine and metadata JSON construction are well-structured

The per-connection state, explicit type whitelist in addColumnTypeAndName, and the resume/bookmark logic for doQueryPrefix/doQueryMetadata/querySuffixWithError match the existing JSON query patterns and should behave correctly under buffer pressure and retries. The main dependency is that callers must invoke clear() between requests to reset queryState and related fields, which you handle on the processor side.

core/src/test/java/io/questdb/test/cutlass/http/HttpQueryTestBuilder.java (3)

82-121: Fragmentation chunk size configuration is wired correctly for tests

Propagating forceSendFragmentationChunkSize and forceRecvFragmentationChunkSize into the HTTP server configuration builder gives tests control over artificial fragmentation without affecting default behavior (Integer.MAX_VALUE effectively disables it).

208-317: Test HTTP wiring for SQL validation and ObjHashSet URLs looks consistent

Using ObjHashSet<String> for /upload, /query, /status, and the config-driven paths (including getContextPathSqlValidation() and getContextPathExport()) aligns the test harness with the production server. The SqlValidationProcessor binding shares the same configuration/engine as other processors, so end-to-end tests should exercise the new endpoint realistically.

362-370: Fluent setters for fragmentation sizes are straightforward

The new withForceRecvFragmentationChunkSize and withForceSendFragmentationChunkSize methods follow the existing builder pattern and keep the default behavior intact when unused.

core/src/main/java/io/questdb/cutlass/http/HttpServerConfigurationWrapper.java (1)

39-121: Context-path methods correctly migrated to ObjHashSet<String>

Returning ObjHashSet<String> from the various getContextPath* methods and delegating straight to the underlying HttpFullFatServerConfiguration keeps the wrapper consistent with the rest of the codebase’s move away from ObjList<String>.

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java (3)

431-464: doResumeSend error handling and buffer-bookmarking match existing patterns

The resume loop correctly:

delegates to state.resume(response) based on queryState,

handles SQL/implicit-cast errors by sending a SQL error response,

converts DataUnavailableException into QueryPausedException with a reset to the last bookmark, and

handles NoSpaceLeftInResponseBufferException via chunk flushing or by escalating when a single unit exceeds buffer size.

The final readyForNextRequest(context) ensures the connection is ready once the JSON suffix has been sent.

506-534: URL parsing and empty-query handling look sound

parseUrl() cleanly distinguishes:

missing/empty query parameter (logging and sending a version-sensitive empty-query notice), and

bad UTF‑8 in the query parameter (logging and returning a structured 400 response).

This is in line with the behavior of other HTTP SQL endpoints and avoids attempting to compile invalid input.

545-571: Shared exception JSON helper is a good reuse of existing behavior

sendException() reuses JsonQueryProcessorState.prepareExceptionJson, keeping the error payload format consistent between validation and execution endpoints while letting you vary HTTP status codes. This also centralizes any future changes to the error schema.

core/src/main/java/io/questdb/cutlass/http/HttpFullFatServerConfiguration.java (4)

31-31: LGTM: Import change aligns with collection type migration.

The import change from ObjList to ObjHashSet is consistent with the refactoring throughout this file.

54-56: LGTM: New SQL validation endpoint follows versioned API convention.

The addition of CONTEXT_PATH_VALIDATE with the path /api/v1/sql/validate follows good API versioning practices, consistent with other v1 endpoints like /api/v1/sql/execute.

70-94: LGTM: Accessor methods correctly return predefined constants.

The accessor methods properly delegate to the static constants, providing a clean interface for accessing context paths.

96-98: LGTM: New SQL validation accessor method.

The getContextPathSqlValidation() method correctly returns the CONTEXT_PATH_VALIDATE constant for the new SQL validation endpoint.

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

core/src/main/java/io/questdb/PropServerConfiguration.java

…ror() there could be a better fix - we could bookmark after writing the suffix to response buffer. maybe.

…into vi_sql_validation

bluestreak01 · 2025-11-21T19:20:17Z

@CodeRabbit review the new changes and see if there are regressions

coderabbitai · 2025-11-21T19:23:09Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of failRequest in SqlValidationProcessor
echo "=== Check failRequest implementation ==="
rg -n -A20 'public void failRequest' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check if there are any new error handling paths ==="
rg -n 'catch.*Exception|throw new' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java | head -30

echo ""
echo "=== Check sendException usage and signature ==="
rg -n -B2 -A10 'private void sendException' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check validate0 implementation ==="
rg -n -A30 'public void validate0' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check onRequestComplete for state initialization ==="
rg -n -A25 'public void onRequestComplete' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Look for any TODO or FIXME comments ==="
rg -n 'TODO|FIXME|XXX|HACK' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

Length of output: 5080

🏁 Script executed:

#!/bin/bash
# Check for potential regressions in related processors
echo "=== Compare error handling with JsonQueryProcessor ==="
rg -n -A15 'public void failRequest' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java

echo ""
echo "=== Check if other processors handle null state ==="
rg -n -B3 -A3 'LV\.get.*context' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java | head -20

echo ""
echo "=== Check ExportQueryProcessor for comparison ==="
rg -n -A15 'public void failRequest' core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java

Length of output: 1791

🏁 Script executed:

#!/bin/bash
# Check the state management and lifecycle
echo "=== Check state lifecycle in SqlValidationProcessorState ==="
rg -n 'public void clear|public void close' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

echo ""
echo "=== Check for resource leaks - factory management ==="
rg -n 'clearFactory|factory.*close|factory.*clear' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check circuit breaker usage ==="
rg -n 'circuitBreaker\.' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

Length of output: 349

🏁 Script executed:

#!/bin/bash
# Check configuration and endpoint setup
echo "=== Check HTTP endpoint configuration ==="
rg -n 'getContextPathSqlValidation|CONTEXT_PATH_VALIDATE' core/src/main/java/io/questdb/cutlass/http/HttpFullFatServerConfiguration.java

echo ""
echo "=== Check PropServerConfiguration for validate path ==="
rg -n -B5 -A5 'httpContextPathSqlValidate' core/src/main/java/io/questdb/PropServerConfiguration.java | head -30

echo ""
echo "=== Check if PropertyKey has the validate key ==="
rg -n 'HTTP_CONTEXT.*VALIDATE' core/src/main/java/io/questdb/PropertyKey.java

Length of output: 2191

🏁 Script executed:

#!/bin/bash
# Check the test coverage for the new functionality
echo "=== Check SqlValidationTest ==="
cat core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java | head -100

echo ""
echo "=== Count test methods ==="
rg -n '@Test' core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java

Length of output: 4813

🏁 Script executed:

#!/bin/bash
# Check factory lifecycle and cleanup
echo "=== Check readyForNextRequest implementation ==="
rg -n -A10 'private void readyForNextRequest' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check factory usage and cleanup ==="
rg -n 'factory' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java | head -20

echo ""
echo "=== Check state.clearFactory usage ==="
rg -n 'clearFactory' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check SqlValidationProcessorState clearFactory implementation ==="
rg -n -A10 'public void clearFactory' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 700

🏁 Script executed:

#!/bin/bash
# Check if ExportQueryProcessor exists and how it handles failRequest
echo "=== Check if ExportQueryProcessor.failRequest exists ==="
rg -n 'class ExportQueryProcessor' core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java

echo ""
echo "=== Check ExportQueryProcessor methods ==="
rg -n 'public.*void (failRequest|onRequestComplete)' core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java

echo ""
echo "=== Check what ExportQueryProcessor extends ==="
rg -n 'extends|implements' core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java | head -5

Length of output: 392

🏁 Script executed:

#!/bin/bash
# Check for other potential issues
echo "=== Check if there are any compiler warnings or issues in error messages ==="
rg -n 'getStatusCode|HTTP_INTERNAL_ERROR|HTTP_BAD_REQUEST' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check internalError implementation ==="
rg -n -A15 'private void internalError' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check if there are any unclosed resources ==="
rg -n 'new.*Sink|new.*Buffer' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

Length of output: 1490

🏁 Script executed:

#!/bin/bash
# Check the complete list of test methods to understand coverage
echo "=== List all test method names ==="
rg -n -A1 '@Test' core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java | grep 'public void'

echo ""
echo "=== Check if there are tests for error conditions ==="
rg -n 'assertGet.*error|assertPost.*error|exception|Exception' core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java | head -15

Length of output: 823

🏁 Script executed:

#!/bin/bash
# Final check - compare with JsonQueryProcessor for consistency
echo "=== Check JsonQueryProcessor readyForNextRequest ==="
rg -n -A10 'private void readyForNextRequest' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java

echo ""
echo "=== Check if JsonQueryProcessor clears factory ==="
rg -n 'clearFactory|factory' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessor.java | head -15

Length of output: 1032

🏁 Script executed:

#!/bin/bash
# Search for readyForNextRequest in SqlValidationProcessor
echo "=== Search for readyForNextRequest definition ==="
rg -n 'readyForNextRequest' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check if it's inherited or in state ==="
rg -n 'def.*readyForNextRequest|void readyForNextRequest' core/src/main/java/io/questdb/cutlass/http/processors/

echo ""
echo "=== Check state.of() implementation which handles factory ==="
rg -n -A20 'public boolean of\(' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 977

🏁 Script executed:

#!/bin/bash
# Check if there are any new regressions in error handling
echo "=== Check complete error flow in validate0 ==="
sed -n '198,230p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check complete error flow in resumeSend ==="
sed -n '171,195p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

Length of output: 2594

🏁 Script executed:

#!/bin/bash
# Check if there are integration issues with HTTP server binding
echo "=== Check how SqlValidationProcessor is wired in HttpServer ==="
rg -n -B3 -A3 'sqlValidation.*Builder|SqlValidation' core/src/main/java/io/questdb/cutlass/http/HttpServer.java | head -30

echo ""
echo "=== Check Services.java wiring ==="
rg -n -B3 -A3 'SqlValidation' core/src/main/java/io/questdb/cutlass/Services.java

Length of output: 1880

🏁 Script executed:

#!/bin/bash
# Check the actual state.of() factory management
echo "=== Check full state.of implementation ==="
rg -n -B5 -A30 'public boolean of\(RecordCursorFactory' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 42

🏁 Script executed:

#!/bin/bash
# Find the state.of method
echo "=== Find all methods named 'of' in SqlValidationProcessorState ==="
rg -n 'public.*of\(' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

echo ""
echo "=== Get the complete SqlValidationProcessorState.of method ==="
sed -n '200,260p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 2125

🏁 Script executed:

#!/bin/bash
# Check readyForNextRequest implementation
echo "=== Get readyForNextRequest full implementation ==="
sed -n '495,505p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check if there's any factory cleanup happening ==="
rg -n -B5 -A5 'clearFactory|factory.*close' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 928

🏁 Script executed:

#!/bin/bash
# Check if there's a resource management regression
echo "=== Check state management in resumeSend error paths ==="
sed -n '177,197p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check doResumeSend implementation ==="
rg -n -A40 'private void doResumeSend' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java | head -50

Length of output: 2779

🏁 Script executed:

#!/bin/bash
# Compare with JsonQueryProcessor's state.of pattern
echo "=== Check JsonQueryProcessorState.of signature ==="
rg -n 'public boolean of\(' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java

echo ""
echo "=== Get JsonQueryProcessorState.of implementation ==="
sed -n '200,260p' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java

Length of output: 2076

🏁 Script executed:

#!/bin/bash
# Search more broadly for factory handling in SqlValidationProcessorState
echo "=== Check all methods in SqlValidationProcessorState ==="
rg -n '^    public' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java | head -30

echo ""
echo "=== Check if there's a method that stores factory ==="
rg -n 'recordCursorFactory|factory' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

Length of output: 1462

🏁 Script executed:

#!/bin/bash
# Compare JsonQueryProcessorState for factory management
echo "=== Check JsonQueryProcessorState factory field and methods ==="
rg -n 'private.*recordCursorFactory|public.*boolean of' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java

echo ""
echo "=== Get JsonQueryProcessorState.of method that takes factory ==="
rg -n -A25 'public boolean of\(.*RecordCursorFactory' core/src/main/java/io/questdb/cutlass/http/processors/JsonQueryProcessorState.java

Length of output: 186

🏁 Script executed:

#!/bin/bash
# Check executeNewSelect in SqlValidationProcessor which should use the factory
echo "=== Check executeNewSelect implementation ==="
sed -n '408,420p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check compileAndValidate where factory might be created ==="
rg -n -A30 'private void compileAndValidate' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

Length of output: 2777

🏁 Script executed:

#!/bin/bash
# Final comprehensive check for resource management
echo "=== Check what happens after query execution ==="
sed -n '285,315p' core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

echo ""
echo "=== Check if state stores any factory reference ==="
rg -n 'class SqlValidationProcessorState' -A50 core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java | head -60

Length of output: 5304

@bluestreak01: I've completed a comprehensive review of the new changes. Here's my analysis:

🔴 Critical Regression Found

1. Null State Bug in `failRequest()` - Pattern Copied from Existing Code

Location: SqlValidationProcessor.java:119-129
Severity: 🔴 Critical

The failRequest() method retrieves state but doesn't check for null before passing it to sendException():

public void failRequest(HttpConnectionContext context, HttpException e) ... {
    final SqlValidationProcessorState state = LV.get(context);  // Line 120 - can be null
    final HttpChunkedResponse response = context.getChunkedResponse();
    sendException(
            state,  // Line 123 - passed without null check
            e.getPosition(),
            e.getFlyweightMessage(),
            HTTP_BAD_REQUEST
    );
    response.shutdownWrite();
}

Then at line 488, sendException() immediately dereferences it:

private void sendException(SqlValidationProcessorState state, ...) {
    final HttpConnectionContext context = state.getHttpConnectionContext();  // NPE if state is null

Root Cause: This pattern was copied from JsonQueryProcessor.java:263, which has the identical bug. This represents a systemic pattern regression across HTTP processors.

Impact: If HTTP parsing fails before onRequestComplete() creates the state (line 146-150), a NullPointerException will crash the request handler.

Recommendation:

public void failRequest(HttpConnectionContext context, HttpException e) ... {
    final SqlValidationProcessorState state = LV.get(context);
    if (state == null) {
        // Early failure before state initialization
        final HttpChunkedResponse response = context.getChunkedResponse();
        JsonQueryProcessor.header(response, context, configuration.getKeepAliveHeader(), HTTP_BAD_REQUEST);
        response.putAscii("{\"error\":\"").putUtf8(e.getFlyweightMessage()).putAscii("\"}");
        response.sendChunk(true);
        return;
    }
    sendException(state, e.getPosition(), e.getFlyweightMessage(), HTTP_BAD_REQUEST);
    context.getChunkedResponse().shutdownWrite();
}

✅ Positive Findings

1. Resource Management - Correctly Implemented ✓

Location: SqlValidationProcessorState.java:374-375

The factory is properly closed using try-with-resources:

boolean of(RecordCursorFactory factory) throws ... {
    try (factory) {  // Auto-closes after metadata extraction
        final RecordMetadata metadata = factory.getMetadata();
        // ... extract metadata ...

This is better than storing the factory and closing it later. No resource leak regression here.

2. Context Binding Fixed ✓

Location: SqlValidationProcessor.java:290-297

The sqlExecutionContext.with() binding is now correctly placed in compileAndValidate() before compilation, addressing the previous issue.

⚠️ Observations

1. Test Coverage: 59.32%

5 test methods covering basic validation scenarios
Missing edge case: failRequest() with null state
Missing test: early HTTP parsing failure

Recommendation: Add test:

@Test
public void testFailRequestBeforeStateInitialization() throws Exception {
    // Simulate HTTP parse error before onRequestComplete creates state
    // Verify graceful error handling without NPE
}

2. Configuration Still Hardcoded

Location: PropServerConfiguration.java:1075

The validation path remains hardcoded:

httpContextPathSqlValidate.add(httpContextWebConsole + "/api/v1/sql/validate");

Unlike other endpoints (exec, import, export) which use PropertyKey for configuration override, the validate path cannot be customized. This may be intentional for a new REST API but is inconsistent with existing patterns.

Summary

1 Critical Bug: Null state handling in failRequest() (copied pattern from existing code)
0 New Regressions: Resource management and context binding are correct
Test Coverage: Adequate but missing edge cases
Recommendation: Fix the null guard before merge to prevent production NPEs

Would you like me to help fix the failRequest() null guard issue?

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java

core/src/main/java/io/questdb/PropServerConfiguration.java

core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessorState.java

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java

core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java

core/src/main/java/io/questdb/griffin/SqlOptimiser.java

core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java

glasstiger · 2025-11-24T13:40:54Z

[PR Coverage check]

😍 pass : 666 / 831 (80.14%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/cutlass/pgwire/PGCleartextPasswordAuthenticator.java	0	1	00.00%
🔵	io/questdb/cutlass/pgwire/PGConnectionContext.java	1	2	50.00%
🔵	io/questdb/cutlass/http/processors/SqlValidationProcessor.java	140	236	59.32%
🔵	io/questdb/cutlass/http/processors/ExportQueryProcessor.java	33	43	76.74%
🔵	io/questdb/cutlass/http/processors/SqlValidationProcessorState.java	168	207	81.16%
🔵	io/questdb/griffin/SqlCompilerImpl.java	45	50	90.00%
🔵	io/questdb/cutlass/http/processors/JsonQueryProcessor.java	32	35	91.43%
🔵	io/questdb/cutlass/http/processors/JsonQueryProcessorState.java	119	129	92.25%
🔵	io/questdb/cutlass/http/HttpResponseSink.java	8	8	100.00%
🔵	io/questdb/cutlass/http/client/AbstractChunkedResponse.java	6	6	100.00%
🔵	io/questdb/griffin/SqlExecutionContextImpl.java	5	5	100.00%
🔵	io/questdb/cutlass/http/WaitProcessor.java	1	1	100.00%
🔵	io/questdb/network/NoSpaceLeftInResponseBufferException.java	3	3	100.00%
🔵	io/questdb/griffin/SqlOptimiser.java	2	2	100.00%
🔵	io/questdb/HttpClientConfiguration.java	1	1	100.00%
🔵	io/questdb/cutlass/http/HttpFullFatServerConfiguration.java	40	40	100.00%
🔵	io/questdb/PropServerConfiguration.java	15	15	100.00%
🔵	io/questdb/cutlass/http/HttpServer.java	7	7	100.00%
🔵	io/questdb/cutlass/http/HttpServerConfiguration.java	6	6	100.00%
🔵	io/questdb/std/Rnd.java	6	6	100.00%
🔵	io/questdb/cutlass/http/processors/ExportQueryProcessorState.java	17	17	100.00%
🔵	io/questdb/cutlass/Services.java	2	2	100.00%
🔵	io/questdb/cutlass/http/client/HttpClient.java	9	9	100.00%

bluestreak01 added 5 commits November 11, 2025 03:45

wip

2d7bc8e

Merge remote-tracking branch 'origin/master' into vi_sql_validation

c1adeb8

merge fallout

a4e951f

Merge remote-tracking branch 'origin/master' into vi_sql_validation

d0aa155

wip

90b7089

bluestreak01 added 9 commits November 11, 2025 19:45

simple test

1d66763

refactoring

a7791f7

Merge remote-tracking branch 'origin/master' into vi_sql_validation

9931ade

added tests, removed file watcher

2e95b3b

defaults!

fc36cb7

hash set instead of lists with duplicate context names

dd43847

fix most tests

c3db491

fix test

fb5d2f3

Merge remote-tracking branch 'origin/master' into vi_sql_validation

647edef

bluestreak01 added the New feature Feature requests label Nov 16, 2025

formattin

2769f37

coderabbitai bot reviewed Nov 16, 2025

View reviewed changes

bluestreak01 and others added 10 commits November 17, 2025 02:01

wip

0b78205

wip

9b83a6c

fix tests

bd07951

Merge remote-tracking branch 'origin/master' into vi_sql_validation

22be8db

fix fragmentation when sending the last chunk after querySuffixWithEr…

57b83a0

…ror() there could be a better fix - we could bookmark after writing the suffix to response buffer. maybe.

fix sql validator error state

e2cf2c6

fuzz tests for /execute

e985365

tested validation end-point to death

e05d7fb

Merge branch 'vi_sql_validation' of https://github.com/questdb/questdb …

196f5dc

…into vi_sql_validation

Merge branch 'master' into vi_sql_validation

fc10d90

jerrinot and others added 4 commits November 21, 2025 17:06

Merge remote-tracking branch 'refs/remotes/origin/vi_sql_validation' …

cf7c13f

…into vi_sql_validation

Merge branch 'master' into vi_sql_validation

2653296

Fix typos

0fa1c95

cleanup

33fd96c

nwoolmer reviewed Nov 21, 2025

View reviewed changes

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessor.java Show resolved Hide resolved

Merge branch 'master' into vi_sql_validation

e7a5298

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/main/java/io/questdb/PropServerConfiguration.java Show resolved Hide resolved

core/src/main/java/io/questdb/PropServerConfiguration.java Show resolved Hide resolved

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessorState.java Outdated Show resolved Hide resolved

puzpuzpuz added 6 commits November 24, 2025 11:17

Fix typo

ede0e5a

Use a sink for error message

a4b7004

Clean up

65d584e

Change redundant put calls to putAscii

ec2808c

One more typo

bda1e74

More putAscii calls

5b41e33

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/main/java/io/questdb/cutlass/http/processors/SqlValidationProcessorState.java Outdated Show resolved Hide resolved

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java Show resolved Hide resolved

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/main/java/io/questdb/griffin/SqlOptimiser.java Show resolved Hide resolved

puzpuzpuz reviewed Nov 24, 2025

View reviewed changes

core/src/test/java/io/questdb/test/cutlass/http/SqlValidationTest.java Show resolved Hide resolved

bluestreak01 added 3 commits November 24, 2025 11:33

address PR comments

45cc3aa

Merge remote-tracking branch 'origin/master' into vi_sql_validation

a3ba7e7

address PR comments, part 2

487cbee

puzpuzpuz self-requested a review November 24, 2025 13:40

puzpuzpuz approved these changes Nov 24, 2025

View reviewed changes

bluestreak01 merged commit 6b60f1d into master Nov 24, 2025
41 checks passed

bluestreak01 deleted the vi_sql_validation branch November 24, 2025 15:33

coderabbitai bot mentioned this pull request Nov 25, 2025

fix(core): breaking change 💥 - remove tx scoreboard v1 #6449

Merged

coderabbitai bot mentioned this pull request Jan 20, 2026

chore(core): remove exception-based query pause mechanism #6670

Merged

Conversation

bluestreak01 commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Uh oh!

coderabbitai bot commented Nov 11, 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

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

bluestreak01 commented Nov 16, 2025

Uh oh!

coderabbitai bot commented Nov 16, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluestreak01 commented Nov 21, 2025

Uh oh!

coderabbitai bot commented Nov 21, 2025

🔴 Critical Regression Found

1. Null State Bug in failRequest() - Pattern Copied from Existing Code

✅ Positive Findings

1. Resource Management - Correctly Implemented ✓

2. Context Binding Fixed ✓

⚠️ Observations

1. Test Coverage: 59.32%

2. Configuration Still Hardcoded

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glasstiger commented Nov 24, 2025

[PR Coverage check]

file detail

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bluestreak01 commented Nov 11, 2025 •

edited

Loading

coderabbitai bot commented Nov 11, 2025 •

edited

Loading

1. Null State Bug in `failRequest()` - Pattern Copied from Existing Code