Jackie-Jiang commented on code in PR #8872:
URL: https://github.com/apache/pinot/pull/8872#discussion_r899520561
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableImplV4.java:
##########
@@ -48,7 +50,26 @@ public DataTableImplV4(int numRows, DataSchema dataSchema,
Map<String, Map<Integ
}
@Override
- protected int getDataBlockVersionType() {
+ public RoaringBitmap getNullRowIds(int colId) {
+ // _fixedSizeData stores two ints per col's null bitmap: offset, and
length.
Review Comment:
Move the implementation into the `BaseDataBlock`
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -289,7 +304,7 @@ public static ColumnarDataBlock
buildFromColumns(List<Object[]> columns, DataSch
}
private static RowDataBlock buildRowBlock(DataBlockBuilder builder) {
- return new RowDataBlock(builder._numRows, builder._dataSchema,
builder._reverseDictionaryMap,
+ return new DataTableImplV4(builder._numRows, builder._dataSchema,
builder._reverseDictionaryMap,
Review Comment:
(minor) Should this be
```suggestion
return new RowDataBlock(builder._numRows, builder._dataSchema,
builder._reverseDictionaryMap,
```
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -164,7 +170,7 @@ public void testAllDataTypes()
}
@Test
- public void testV3V4Compatibility()
+ public void testV2V3Compatibility()
Review Comment:
Can we revert the reordering of V2V3 and V3V4 compatibility test? It is
really hard to track the changes here
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -659,44 +849,57 @@ private void fillDataTableWithRandomData(DataTableBuilder
dataTableBuilder,
}
dataTableBuilder.finishRow();
}
+ if (nullBitmaps != null) {
+ for (int colId = 0; colId < numColumns; colId++) {
+ System.out.println("col ID: " + colId);
Review Comment:
Remove this
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -535,56 +574,207 @@ public void testDataTableMetadataBytesLayout()
}
}
+ private RowDataBlock getDataBlockFromRandomData(DataSchema schema, int
numColumns)
Review Comment:
I don't think this is needed. We should use `fillDataTableWithRandomData()`
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -426,6 +429,40 @@ public void testV2V3Compatibility()
Assert.assertEquals(newDataTable.getMetadata(), EXPECTED_METADATA);
}
+ @Test
+ public void testDataBlockBuilderV4()
Review Comment:
We should test `DataTableBuilder` here. `DataBlockBuilder` is used only for
multi-stage engine, which is out of the scope of this test
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableImplV4.java:
##########
@@ -48,7 +50,26 @@ public DataTableImplV4(int numRows, DataSchema dataSchema,
Map<String, Map<Integ
}
@Override
- protected int getDataBlockVersionType() {
+ public RoaringBitmap getNullRowIds(int colId) {
+ // _fixedSizeData stores two ints per col's null bitmap: offset, and
length.
+ int position = _numRows * _rowSizeInBytes + colId * Integer.BYTES * 2;
+ _fixedSizeData.position(position);
Review Comment:
We should check the length of the buffer. If `nullRowIds` are not passed,
fixed size data length will be smaller than the position, and we should return
`null` in that scenario
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
