[GLUTEN-9205][CH] Support deletion vector native write#9248
[GLUTEN-9205][CH] Support deletion vector native write#9248loneylee merged 9 commits intoapache:mainfrom
Conversation
|
Run Gluten Clickhouse CI on x86 |
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨No code suggestions found for the PR. |
There was a problem hiding this comment.
Copilot reviewed 11 out of 15 changed files in this pull request and generated no comments.
Files not reviewed (4)
- backends-clickhouse/src-delta-32/main/scala/org/apache/gluten/sql/shims/delta32/Delta32Shims.scala: Language not supported
- backends-clickhouse/src-delta-32/main/scala/org/apache/spark/sql/execution/DeletionVectorWriteTransformer.scala: Language not supported
- backends-clickhouse/src-delta-32/test/scala/org/apache/spark/gluten/delta/GlutenDeltaParquetDeletionVectorSuite.scala: Language not supported
- backends-clickhouse/src-delta/main/scala/org/apache/gluten/component/CHDeltaComponent.scala: Language not supported
Comments suppressed due to low confidence (1)
cpp-ch/local-engine/Storages/SubstraitSource/Delta/DeltaWriter.cpp:95
- Ensure that 'get64(0)' is intended to always fetch the first row's value; if each row should have its own cardinality, consider using the current row index (e.g., get64(row_idx)).
auto cardinality = cardinality_src_columns.column->get64(0); // alisa deletedRowIndexCount
|
Run Gluten Clickhouse CI on x86 |
PR Code Suggestions ✨No code suggestions found for the PR. |
|
Run Gluten Clickhouse CI on x86 |
| /** | ||
| * Contains utility classes and method for performing DML operations with Deletion Vectors. | ||
| */ | ||
| object DMLWithDeletionVectorsHelper extends DeltaCommand { |
There was a problem hiding this comment.
This scala class is copied from the delta source, please ref to the object VacuumCommand, add some comments for the contents of the changes which is different from the source, and add the comments // --- modified start and // --- modified end to specify the changes.
| public static native long deletionVectorWriteFinalize(long writer_address); | ||
|
|
||
| public static String encodeUUID(String uuid, String randomPrefix) { | ||
| return DeletionVectorWriteTransformer.encodeUUID(uuid, randomPrefix); |
There was a problem hiding this comment.
call the java api from the native to generate the uuid ?
There was a problem hiding this comment.
Yes, comment has been added
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Copilot reviewed 11 out of 16 changed files in this pull request and generated 1 comment.
Files not reviewed (5)
- backends-clickhouse/src-delta-32/main/scala/org/apache/gluten/sql/shims/delta32/Delta32Shims.scala: Language not supported
- backends-clickhouse/src-delta-32/main/scala/org/apache/spark/sql/execution/DeletionVectorWriteTransformer.scala: Language not supported
- backends-clickhouse/src-delta-32/test/scala/org/apache/spark/gluten/delta/GlutenDeltaParquetDeletionVectorSuite.scala: Language not supported
- backends-clickhouse/src-delta/main/scala/org/apache/gluten/component/CHDeltaComponent.scala: Language not supported
- backends-clickhouse/src-delta/main/scala/org/apache/gluten/sql/shims/DeltaShims.scala: Language not supported
Comments suppressed due to low confidence (1)
cpp-ch/local-engine/Storages/SubstraitSource/Delta/DeltaWriter.cpp:247
- Verify that passing a std::byte value to writeIntBinary is intended; an explicit cast to an integer type might be required to ensure correct binary output.
DB::writeIntBinary(DV_FILE_FORMAT_VERSION_ID_V1, *write_buffer);
| auto cardinality = cardinality_src_columns.column->get64(row_idx); // alisa deletedRowIndexCount | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider correcting the comment from 'alisa' to 'alias' for clarity.
| auto cardinality = cardinality_src_columns.column->get64(row_idx); // alisa deletedRowIndexCount | |
| auto cardinality = cardinality_src_columns.column->get64(row_idx); // alias deletedRowIndexCount |
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
(Fixes: #9205)
How was this patch tested?
test by ut