fix(sql): fix parquet export bug when SQL contains now() function#6499
fix(sql): fix parquet export bug when SQL contains now() function#6499bluestreak01 merged 2 commits intomasterfrom
now() function#6499Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe change captures and propagates the current timestamp context (now and nowTimestampType) through the Parquet export pipeline, from initial query processing to downstream execution context setup. Two new fields are added to CopyExportRequestTask with corresponding accessors, and the values are threaded through ExportQueryProcessor, CopyExportRequestJob, and CopyExportFactory to SerialParquetExporter, where the SQL execution context is updated with the captured timestamp. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
kafka1991
changed the title
now() function in parquet exportnow() function
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java (1)
43-45: Task-levelnowcontext wiring is consistent; consider clearing inclear()The added
now/nowTimestampTypefields, their getters, and the extendedof(...)signature cleanly carry timestamp context through the export pipeline and are used consistently by all shown call sites (ExportQueryProcessor,CopyExportFactory,CopyExportRequestJob,SerialParquetExporter).One small robustness improvement would be to reset
nowandnowTimestampTypeinclear(), alongside the other fields, so that a reused instance can’t accidentally expose stale values if someone ever accesses these getters before callingof(...):public void clear() { this.entry = null; this.tableName = null; this.fileName = null; this.compressionCodec = -1; this.compressionLevel = -1; this.dataPageSize = -1; this.parquetVersion = -1; this.rowGroupSize = -1; this.statisticsEnabled = true; + this.now = 0; + this.nowTimestampType = 0; this.createOp = Misc.free(createOp); result = null; }Also applies to: 99-106, 135-150, 163-165
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java(2 hunks)core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestJob.java(1 hunks)core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestTask.java(4 hunks)core/src/main/java/io/questdb/cutlass/parquet/SerialParquetExporter.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/ops/CopyExportFactory.java(2 hunks)core/src/test/java/io/questdb/test/cutlass/http/ExpParquetExportTest.java(2 hunks)core/src/test/java/io/questdb/test/griffin/CopyExportTest.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/src/test/java/io/questdb/test/griffin/CopyExportTest.java (1)
core/src/main/java/io/questdb/cairo/MicrosTimestampDriver.java (1)
MicrosTimestampDriver(69-1609)
⏰ 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 (Coverage Report Coverage Report)
- 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 (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- 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 (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- 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 (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (6)
core/src/main/java/io/questdb/cutlass/parquet/SerialParquetExporter.java (1)
103-110: Seeding exporter SqlExecutionContext with capturednowlooks correctCalling
sqlExecutionContext.setNowAndFixClock(task.getNow(), task.getNowTimestampType())before any SQL work inprocess()correctly alignsnow()evaluation in the exporter with the value captured upstream in the task. This directly addresses the bug around uninitialised or driftingnow()during Parquet export.core/src/test/java/io/questdb/test/cutlass/http/ExpParquetExportTest.java (1)
30-30: HTTP/expnow()-based Parquet export test is well integratedThe added
testExportWithNowParameterplus theColumnTypeimport give good coverage that/expParquet exports respect a fixednow()(via microsecond clock +setNowAndFixClock) when filtering onts < now(). The pattern matches existing tests and exercises the new timestamp plumbing end‑to‑end.Also applies to: 468-479
core/src/test/java/io/questdb/test/griffin/CopyExportTest.java (1)
29-30: SQL COPY now()-based Parquet export scenario is well covered
testCopyWithNowParametercorrectly fixescurrentMicroswithMicrosTimestampDriver.floor(...), runs aCOPY (SELECT ... WHERE ts < now())to Parquet, and verifies only the expected row is present in the exported file. This gives solid regression coverage for the bug this PR targets and aligns with existing copy-export test patterns.Also applies to: 1311-1335
core/src/main/java/io/questdb/griffin/engine/ops/CopyExportFactory.java (1)
167-185: Propagatingnowfrom execution context into queued export task is appropriateCapturing
nowTimestampTypeandnowfrom theSqlExecutionContextright before enqueuing theCopyExportRequestTask, and passing them intotask.of(...), ensures the asynchronous COPY-to-Parquet worker sees the same fixednow()value as the original COPY statement. This matches the HTTP path and is a clean extension of the existing task payload.core/src/main/java/io/questdb/cutlass/http/processors/ExportQueryProcessor.java (1)
595-618: HTTP export path correctly threadsnowinto the Parquet export taskThe added
nowTimestampType/nowcapture incopyQueryToParquetFile()and their inclusion intask.of(...)align the/expParquet export with theSqlExecutionContextthat compiled the query (already initialised viainitNow()). Together withSerialParquetExporter.setNowAndFixClock(...), this should eliminate discrepancies innow()evaluation between query compilation and asynchronous export.core/src/main/java/io/questdb/cutlass/parquet/CopyExportRequestJob.java (1)
80-99: Worker’s local task copy now preservesnowcontextExtending
localTaskCopy.of(...)to includerawArrayEncoding,nowTimestampType, andnowmirrors the queue task’s state and guarantees thatSerialParquetExportersees the same timestamp context that was captured when the request was enqueued.
|
@kafka1991 there is an actionable nit-pick from the robot! |
Hi @bluestreak01 Thanks! the nit has been resolved. |
[PR Coverage check]😍 pass : 15 / 15 (100.00%) file detail
|
3 participants
When exporting a query that contains the
now()function to Parquet, thenow()value was not initialized before query execution, leading to incorrect export results.