chore(core): SymbolMapWriter nullFlag not maintained when column is not included in INSERT#6645
chore(core): SymbolMapWriter nullFlag not maintained when column is not included in INSERT#6645bluestreak01 merged 2 commits intomasterfrom
SymbolMapWriter nullFlag not maintained when column is not included in INSERT#6645Conversation
…ncluded in INSERT
WalkthroughAdds symbol null flag change tracking across the WAL stack (TableWriter, WalWriter, WalEventWriter, ViewWalWriter). Introduces new dedup removal counters and a dedicated WAL commit pathway. Extends configureNullSetters signature to include columnIndex and symbol writer references. Adds test cases for latest-by queries with null symbols. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@coderabbitai 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: 3
🤖 Fix all issues with AI agents
In `@core/src/main/java/io/questdb/cairo/TableWriter.java`:
- Around line 783-786: The method addPhysicallyWrittenRows currently accepts any
long and can record negative values; change it to guard against negatives by
returning early (or ignoring) when rows <= 0 before updating
physicallyWrittenRowsSinceLastCommit and calling
metrics.tableWriterMetrics().addPhysicallyWrittenRows(rows); ensure you
reference the existing method addPhysicallyWrittenRows and the fields
physicallyWrittenRowsSinceLastCommit and
metrics.tableWriterMetrics().addPhysicallyWrittenRows in the change so only
positive row counts are recorded.
- Around line 728-730: The addDedupRowsRemoved method currently accepts negative
counts which can corrupt telemetry; update addDedupRowsRemoved(long count) to
validate the input and reject negatives: if count < 0, throw an
IllegalArgumentException (with a clear message including the invalid count)
instead of calling dedupRowsRemovedSinceLastCommit.add(count); otherwise proceed
to add the count. Ensure the exception message references addDedupRowsRemoved
and the provided count so callers can diagnose the issue.
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/cairo/TableWriter.java (1)
3159-3241: Fix is on the right path: SYMBOL null setter now updatesSymbolMapWriter.nullFlag.
This should correctly cover implicit nulls fromrow.append()when the caller didn’t touch the SYMBOL column. One small nit: callingupdateNullFlag(true)on every null row might be redundant; ifupdateNullFlagisn’t already idempotent/cheap, consider a cheap internal guard (only if you have a fast “already true” check available).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/io/questdb/cairo/TableWriter.javacore/src/main/java/io/questdb/cairo/wal/ViewWalWriter.javacore/src/main/java/io/questdb/cairo/wal/WalEventWriter.javacore/src/main/java/io/questdb/cairo/wal/WalWriter.javacore/src/test/java/io/questdb/test/griffin/LatestByTest.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-07T00:59:31.522Z
Learnt from: bluestreak01
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-07T00:59:31.522Z
Learning: In QuestDB's Cairo engine, transaction (_txn) files have a strong invariant: they are never truncated below TX_BASE_HEADER_SIZE. Once created, they are either fully formed (size >= header size) or completely removed along with the entire table directory when the table is dropped.
Applied to files:
core/src/main/java/io/questdb/cairo/TableWriter.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 (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 (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- 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 (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-arm64)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- 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 (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo 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)
- GitHub Check: build
🔇 Additional comments (16)
core/src/main/java/io/questdb/cairo/TableWriter.java (4)
205-205: Good:LongAdderfor dedup removal counter (thread-safe for async tasks).
This matches howphysicallyWrittenRowsSinceLastCommitis tracked and should tolerate concurrent increments from O3/WAL worker tasks.
2265-2267: Nice: exported getter for dedup removals since last commit.
This will be useful for WAL apply observability/metrics.
2301-2304: No concerns.
4171-4173: Good: null-setter configuration now passescolumnIndex+symbolMapWriters.
This wires the SYMBOL null-flag behavior consistently across base + both O3 setter lists.core/src/test/java/io/questdb/test/griffin/LatestByTest.java (2)
276-296: Good test coverage for the null symbol flag fix.This test correctly validates the fix by inserting rows with only the timestamp column populated (implicitly nullifying the symbol columns), then verifying that
LATEST ONqueries properly handle these null symbols. The expected output correctly shows two rows: one for the null symbol combination and one for the non-null symbols.
298-319: Good WAL-specific test coverage.This test mirrors the non-WAL test but explicitly exercises the WAL code path where the
symbolMapNullFlagsChangedtracking is critical. ThedrainWalQueue()call ensures the WAL events are processed before verification.core/src/main/java/io/questdb/cairo/wal/ViewWalWriter.java (1)
71-71: Correct signature update for view writer.The additional
nullargument aligns with the extendedWalEventWriter.of()signature. Since views don't contain symbol columns, passingnullfor all symbol-related parameters is appropriate.core/src/main/java/io/questdb/cairo/wal/WalEventWriter.java (2)
444-449: Clean API extension.The
of()method properly accepts and stores the newsymbolMapNullFlagsChangedparameter, maintaining consistency with how other symbol-related state lists are handled.
238-264: Core fix logic is correct.The updated condition at line 244 now properly triggers symbol map diff writing when
symbolMapNullFlagsChanged.get(columnIndex)is true. This ensures the null flag state is persisted to WAL even when no new symbol values are added—which is the essence of this bug fix.The field
symbolMapNullFlagsChangedis guaranteed to be non-null whenwriteSymbolMapDiffs()executes. The method is only called fromappendData(), which is only invoked byWalWriter(notViewWalWriter).ViewWalWriterpasses null values to theof()method but never callsappendData(), so no null safety issue exists.core/src/main/java/io/questdb/cairo/wal/WalWriter.java (7)
128-128: New tracking field for symbol null flag changes.The
symbolMapNullFlagsChangedlist correctly tracks which symbol columns have had their null flag transition fromfalsetotrueduring the current transaction.
592-600: Correct null setter implementation for symbols.The lambda properly:
- Writes
VALUE_IS_NULLto the data column- Only marks
symbolMapNullFlagsChangedastruewhen transitioning from a non-null stateThe conditional check
!symbolMapNullFlags.get(columnIndex)prevents redundant flag updates when multiple null values are inserted for the same column within a transaction.
931-931: Call site updated correctly.The
configureNullSettersinvocation now passes the requiredcolumnIndex,symbolMapNullFlagsChanged, andsymbolMapNullFlagsparameters.
1333-1333: Segment rollover correctly resets the change tracking flag.Resetting
symbolMapNullFlagsChangedtofalsewhen opening a new segment ensures that each segment's WAL events only capture the null flag changes that occurred within that segment.
2630-2633: Explicit null symbol handling correctly tracks flag changes.This handles the case where
putSym(columnIndex, null)is called explicitly. The logic correctly:
- Checks if the null flag isn't already set
- Only then marks both the current state and the change flag
Minor note: This code path uses
set()while the null setter lambda (line 596-597) usessetQuick(). Both work correctly since the lists are pre-sized, butsetQuick()would be marginally more consistent.
190-190: Event writer correctly initialized with null flag tracking.The
WalEventWriteris properly wired with all symbol-related lists, including the newsymbolMapNullFlagsChangedtracking list.
1451-1451: Transaction boundary correctly resets change tracking.After a commit,
resetSymbolMapsproperly resetssymbolMapNullFlagsChangedtofalsewhile preserving the actualsymbolMapNullFlagsstate from the reader. This ensures subsequent transactions only track new changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
[PR Coverage check]😍 pass : 81 / 85 (95.29%) file detail
|
bluestreak01
changed the title
SymbolMapWriter nullFlag not maintained when column is not included in INSERTSymbolMapWriter nullFlag not maintained when column is not included in INSERT
3 participants
Fixed an issue where
SymbolMapWriter.nullFlagwas not set to true when Symbol column values were implicitly set to null (via row.append() without explicit value).