You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/08 15:44:48 UTC
[GitHub] [pinot] walterddr opened a new pull request, #9758: [hotfix] cover non-tested data table v4 code path
walterddr opened a new pull request, #9758:
URL: https://github.com/apache/pinot/pull/9758
At first I thought datatable version has been defaulted to v4 when committing #9731 . but apparently it is not even tested.
created this hotfix follow up as well as adding abundant tests.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] agavra commented on a diff in pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9758:
URL: https://github.com/apache/pinot/pull/9758#discussion_r1016888720
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java:
##########
@@ -62,12 +62,23 @@ public class DataTableImplV4 extends BaseDataTable {
protected byte[] _variableSizeDataBytes;
protected ByteBuffer _variableSizeData;
protected Map<String, String> _metadata;
- private static final int VERSION = 1;
protected int[] _columnOffsets;
protected int _rowSizeInBytes;
public DataTableImplV4() {
- super();
+ _numRows = 0;
Review Comment:
not sure I can tell from just reviewing the PR, what was the bug?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9758:
URL: https://github.com/apache/pinot/pull/9758#issuecomment-1307660787
> Since this is labelled as hotfix, I am wondering if you have started using V4.
>
> AFAIK, it wasn't tested and there was plan to move over to V4 since it also has NULL handling support IIRC. The current / default version is V3.
label as hotfix because this was a regression introduced a bit earlier:
- when i decouple the code in #9731 , the DataTableImplV4 becomes a copy of the original BaseDataBlock, but since it is no longer a data block, we dont have the test coverage via the DataBlockTest, and neither was it tested in the DataTableTest b/c it doesn't test V4 (only V3)
- it is not being used after the decoupling, but it is still good to test the V4 impl.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9758:
URL: https://github.com/apache/pinot/pull/9758#discussion_r1016897609
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -100,9 +101,10 @@ public class DataTableSerDeTest {
.put(MetadataKey.NUM_RESIZES.getName(), String.valueOf(900L))
.put(MetadataKey.RESIZE_TIME_MS.getName(), String.valueOf(1919199L)).build();
- @Test
- public void testException()
+ @Test(dataProvider = "versionProvider")
Review Comment:
version 4 was never tested.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9758:
URL: https://github.com/apache/pinot/pull/9758#discussion_r1016899198
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java:
##########
@@ -62,12 +62,23 @@ public class DataTableImplV4 extends BaseDataTable {
protected byte[] _variableSizeDataBytes;
protected ByteBuffer _variableSizeData;
protected Map<String, String> _metadata;
- private static final int VERSION = 1;
protected int[] _columnOffsets;
protected int _rowSizeInBytes;
public DataTableImplV4() {
- super();
+ _numRows = 0;
Review Comment:
see my previous comment :-)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9758:
URL: https://github.com/apache/pinot/pull/9758#discussion_r1016898343
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java:
##########
@@ -46,7 +46,7 @@
* Datatable V4 Implementation is a wrapper around the Row-based data block.
*/
@InterfaceStability.Evolving
-public class DataTableImplV4 extends BaseDataTable {
+public class DataTableImplV4 implements DataTable {
Review Comment:
this class was copied from BaseDataBlock, but it wasn't extending BaseDataTable previously, thus some of the protected fields in BaseDataTable is not initialized.
this PR changes it back to implementing the interface and thus make the impl clean
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9758:
URL: https://github.com/apache/pinot/pull/9758#issuecomment-1307647500
Since this is labelled as hotfix, I am wondering if you have started using V4.
AFAIK, it wasn't tested and there was plan to move over to V4 since it also has NULL handling support IIRC. The current / default version is V3.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9758:
URL: https://github.com/apache/pinot/pull/9758#discussion_r1016897609
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -100,9 +101,10 @@ public class DataTableSerDeTest {
.put(MetadataKey.NUM_RESIZES.getName(), String.valueOf(900L))
.put(MetadataKey.RESIZE_TIME_MS.getName(), String.valueOf(1919199L)).build();
- @Test
- public void testException()
+ @Test(dataProvider = "versionProvider")
Review Comment:
version 4 is never tested.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr merged pull request #9758: [hotfix] cover non-tested data table v4 code path
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9758:
URL: https://github.com/apache/pinot/pull/9758
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org