Spark 4.1: Fix row lineage in vectorized ORC reads - #17635
Conversation
Co-authored-by: Joy Haldar <joy.haldar@target.com>
|
cc: @pvary, @Guosmilesmile, please take a look when you have some time. |
Guosmilesmile
left a comment
Guosmilesmile
left a comment
There was a problem hiding this comment.
Thank you very much for the PR! Could we add an end-to-end test case as well?
| }; | ||
| } | ||
|
|
||
| private static Map<Integer, Integer> buildFieldIdToOrcIndex(TypeDescription orcType) { |
There was a problem hiding this comment.
Seem the same in orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java . Can we extract common part into a shared helper？
|
|
||
| GenericRecord record = GenericRecord.create(writeSchema); | ||
| ImmutableList.Builder<Record> builder = ImmutableList.builder(); | ||
| for (int i = 0; i < 4; i++) { |
There was a problem hiding this comment.
We currently only test within a batch. Could we add a test case where batchOffsetInFile > 0 to cover the firstRowId + batchOffsetInFile + rowId logic?
| orcType.getChildren().size()); | ||
| } | ||
|
|
||
| private ColumnVector storedVector( |
There was a problem hiding this comment.
Should these private methods be moved to the bottom, after the public methods?
| } | ||
|
|
||
| @Test | ||
| public void testRowLineage() throws IOException { |
There was a problem hiding this comment.
We should avoid using test prefixes for newly added tests.
| } | ||
|
|
||
| @Test | ||
| public void testRowLineage() throws IOException { |
There was a problem hiding this comment.
May be package private？
| import org.apache.spark.sql.vectorized.ColumnarMap; | ||
| import org.apache.spark.unsafe.types.UTF8String; | ||
|
|
||
| public class LastUpdatedSeqColumnVector extends ColumnVector { |
There was a problem hiding this comment.
There’s quite a bit of duplicated code between RowIdColumnVector and LastUpdatedSeqColumnVector. Could we extract the common logic into a base class?
2 participants
VectorizedSparkOrcReaders.StructConverter checked
idToConstantbefore any metadata column, so_row_idmatched the generic constant branch and every row receivedfirst_row_id. Stored per-row values were ignored and the position offset was never applied._last_updated_sequence_numberhad the same problem._row_idfor a file withfirst_row_id = 100. Rows 0 and 2 store explicit values (555, 557), rows 1 and 3 store null and should inherit100 + position.The fix mirrors what the row-based reader already does in OrcValueReaders: check the metadata columns first, and fall back to
first_row_id + positiononly when the file has no stored value. AddsRowIdColumnVectorandLastUpdatedSeqColumnVectorfor that, and passes the ORC schema into StructConverter so the stored columns can be found by field id.Row based ORC got lineage support in #15776 and #16534, but the vectorized reader was not added.
Found while adding vectorized coverage to the format model TCK (#17610).