-
Notifications
You must be signed in to change notification settings - Fork 2
Prudhvi.spirit bump #3
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.
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
Conversation
- Fix staging to use NonGeneratedColumns instead of Columns when inserting Generated columns cannot be explicitly inserted, they are computed automatically - Add TestStageRunnerWithGeneratedColumns to test staging tables with VIRTUAL and STORED generated columns - Add TestArchiveRunnerWithGeneratedColumns to test archiving to table with generated columns - Add TestArchiveRunnerWithGeneratedColumns_ParquetFile to test archiving to parquet with generated columns - Fix float64Val in parquet writer to handle DECIMAL values returned as []byte from MySQL
f560424 to
3946e0c
Compare
| b.chunker, err = table.NewChunker(sconfig.SrcTbl, sconfig.ChunkDuration, sconfig.Logger) | ||
| // Use slog.Default() for the chunker since spirit now uses slog.Logger | ||
| // Pass SrcTbl as both old and new table since we're archiving from the same table | ||
| b.chunker, err = table.NewChunker(sconfig.SrcTbl, sconfig.SrcTbl, sconfig.ChunkDuration, slog.Default()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @morgo I am forced to pass a non nil table for destination here as the OpenAtWaterMark() for optimisticChunker assumes that the destination table is not nil. As this is is archive phase (in which we copy all the rows from staged table without where clause, it uses optmistic chunker). I am not sure what the right thing to do here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same problem in move table operations. The API expects two tables because it uses this to calculate a column intersection -- but that doesn't make sense for all use cases.
I think what you have is fine for now. I might change the API in future to have something clearer for non-transforming cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thanks, i see the that the highwater mark is mainly used in replication client by KeyAboveHighWatermark and polt doesn't use that method, so should be safe.
Fixes
#1
#2