Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(cairo): fix reader reload after truncate table #2022

Merged
merged 18 commits into from Apr 19, 2022
Merged

fix(cairo): fix reader reload after truncate table #2022

merged 18 commits into from Apr 19, 2022

Conversation

ideoma
Copy link
Collaborator

@ideoma ideoma commented Apr 10, 2022

When table is truncated, TableReader did not truck this in the reload() method and only reload partitions if they were missing. With the fix TableReader checks truncateVersion in _txn file and when changed all fully reloads all the partitions.
Attach and drop partitions forces same full reload on TableReader

@puzpuzpuz puzpuzpuz added Bug Incorrect or unexpected behavior Core Related to storage, data type, etc. labels Apr 12, 2022
@puzpuzpuz puzpuzpuz self-requested a review April 13, 2022 08:38
@ideoma ideoma requested a review from puzpuzpuz April 13, 2022 13:16
puzpuzpuz
puzpuzpuz previously approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a few questions.

core/src/main/java/io/questdb/cairo/TableWriter.java Show resolved Hide resolved
core/src/main/java/io/questdb/cairo/TableWriter.java Outdated Show resolved Hide resolved
core/src/test/java/io/questdb/griffin/AlterTableAttachPartitionTest.java Show resolved Hide resolved
@ideoma ideoma marked this pull request as draft April 14, 2022 12:21
@ideoma ideoma marked this pull request as ready for review April 14, 2022 14:00
puzpuzpuz
puzpuzpuz previously approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding validation for index files!

core/src/main/java/io/questdb/cairo/TableWriter.java Outdated Show resolved Hide resolved
@puzpuzpuz
Copy link
Contributor

I may have overlooked one more place that we might need to update: io.questdb.cliutil.TxSerializer and TxFileStruct classes.

@ideoma ideoma requested a review from puzpuzpuz April 19, 2022 10:10
@ideoma
Copy link
Collaborator Author

ideoma commented Apr 19, 2022

[PR Coverage check]

😍 pass : 205 / 208 (98.56%)

file detail

path covered line new line coverage
🔵 io/questdb/cairo/ColumnVersionWriter.java 7 8 87.50%
🔵 io/questdb/cairo/TableWriter.java 110 112 98.21%
🔵 io/questdb/cairo/TxWriter.java 4 4 100.00%
🔵 io/questdb/cairo/SymbolMapWriter.java 3 3 100.00%
🔵 io/questdb/cairo/TableReader.java 73 73 100.00%
🔵 io/questdb/griffin/SqlCompiler.java 1 1 100.00%
🔵 io/questdb/cairo/TableUtils.java 3 3 100.00%
🔵 io/questdb/cairo/mig/Mig620.java 1 1 100.00%
🔵 io/questdb/cairo/TxReader.java 3 3 100.00%

@bluestreak01 bluestreak01 merged commit 74ea789 into master Apr 19, 2022
@bluestreak01 bluestreak01 deleted the ap-fix-truncate-read branch April 19, 2022 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

Assignees
No one assigned
Labels
Bug Incorrect or unexpected behavior Core Related to storage, data type, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ideoma @puzpuzpuz @bluestreak01