fix(core): fix incorrect designated timestamp column displayed by tables() after column type changes#6570
Conversation
…mp column after column type change
|
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 WalkthroughMetadataCache.MetadataCacheWriterImpl.hydrateTable method now stores sorted columns in a local variable, calls setPosition(i) on each, and updates the table's timestamp index when processing timestamp-designated columns. Two new test cases were added to verify timestamp index behavior after column type changes and column drops. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
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 |
|
@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/test/java/io/questdb/test/cairo/MetadataCacheTest.java (1)
840-848: Useful helper method for test assertions.The
assertTimestamphelper simplifies timestamp index verification in the new tests. The hardcoded timestamp column name "timestamp" is acceptable since it matches the schema of table 'x' used in the current tests.💡 Optional: Make the helper more flexible for future use
If this helper is used with tables having different timestamp column names in the future, consider adding the timestamp name as a parameter:
-private static void assertTimestamp(TableToken tt, int timestampIndex) { +private static void assertTimestamp(TableToken tt, int timestampIndex, String expectedTimestampName) { try (MetadataCacheReader reader = engine.getMetadataCache().readLock()) { final CairoTable table = reader.getTable(tt); Assert.assertNotNull(table); final int tsIndex = table.getTimestampIndex(); Assert.assertEquals(timestampIndex, tsIndex); - Assert.assertEquals("timestamp", table.getTimestampName()); + Assert.assertEquals(expectedTimestampName, table.getTimestampName()); } }Then update the calls:
-assertTimestamp(tt, 3); +assertTimestamp(tt, 3, "timestamp");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/io/questdb/cairo/MetadataCache.java(1 hunks)core/src/test/java/io/questdb/test/cairo/MetadataCacheTest.java(2 hunks)
⏰ 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). (36)
- 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 (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- 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 (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
core/src/main/java/io/questdb/cairo/MetadataCache.java (1)
599-607: LGTM! Critical fix correctly implemented.The addition of the timestamp index update after column sorting correctly addresses the bug described in the PR. When columns are sorted (line 597), the designated timestamp column's logical position may change, especially after column type changes that affect columns positioned before it. The fix ensures the table's timestamp index reflects the post-sort position by updating it during the iteration over sorted columns.
The use of a local variable
columnimproves readability and performance by avoiding repeatedgetQuick(i)calls.core/src/test/java/io/questdb/test/cairo/MetadataCacheTest.java (2)
721-732: Good test coverage for the column type change scenario.This test correctly validates that the designated timestamp index remains stable when a column positioned before it undergoes a type change. The test directly addresses the bug described in the PR objectives.
Since table 'x' is a non-WAL table, the absence of
drainWalQueue()is appropriate—metadata updates are synchronous for non-WAL tables.
734-745: Good complementary test for the column drop scenario.This test validates that the designated timestamp index correctly adjusts when a column positioned before it is dropped. After dropping column 'i' at position 0, the timestamp index correctly decrements from 3 to 2, verifying the post-sort timestamp index update logic in the fix.
bluestreak01
changed the title
tables() after column type changestables() after column type changes
[PR Coverage check]😍 pass : 4 / 4 (100.00%) file detail
|
2 participants
The
tables()function displayed incorrect designated timestamp column when a column type change occurred on any column positioned before the designated timestamp column in the table schema.