You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2023/04/08 04:55:52 UTC
[GitHub] [pinot] xiangfu0 opened a new pull request, #10583: Fix DataTableV3 serde bug for empty array
xiangfu0 opened a new pull request, #10583:
URL: https://github.com/apache/pinot/pull/10583
Caught this exception in DataTableV4 fix, apparently, it happens in V3 as well.
--
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] xiangfu0 commented on a diff in pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10583:
URL: https://github.com/apache/pinot/pull/10583#discussion_r1161151652
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -123,36 +123,142 @@ public void testException(int dataTableVersion)
}
@Test(dataProvider = "versionProvider")
- public void testEmptyStrings(int dataTableVersion)
+ public void testEmptyValues(int dataTableVersion)
throws IOException {
DataTableBuilderFactory.setDataTableVersion(dataTableVersion);
String emptyString = StringUtils.EMPTY;
String[] emptyStringArray = {StringUtils.EMPTY};
+ ByteArray emptyBytes = new ByteArray(new byte[0]);
+ for (int numRows = 0; numRows < NUM_ROWS; numRows++) {
+ testEmptyValues(new DataSchema(new String[]{"STR_SV", "STR_MV"}, new DataSchema.ColumnDataType[]{
Review Comment:
We don't really use `OBJECT` type anywhere and what is an empty `OBJECT` if not null?
--
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] xiangfu0 commented on a diff in pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10583:
URL: https://github.com/apache/pinot/pull/10583#discussion_r1161151724
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -123,36 +123,142 @@ public void testException(int dataTableVersion)
}
@Test(dataProvider = "versionProvider")
- public void testEmptyStrings(int dataTableVersion)
+ public void testEmptyValues(int dataTableVersion)
throws IOException {
DataTableBuilderFactory.setDataTableVersion(dataTableVersion);
String emptyString = StringUtils.EMPTY;
String[] emptyStringArray = {StringUtils.EMPTY};
+ ByteArray emptyBytes = new ByteArray(new byte[0]);
+ for (int numRows = 0; numRows < NUM_ROWS; numRows++) {
+ testEmptyValues(new DataSchema(new String[]{"STR_SV", "STR_MV"}, new DataSchema.ColumnDataType[]{
+ DataSchema.ColumnDataType.STRING, DataSchema.ColumnDataType.STRING_ARRAY
+ }), numRows, new Object[]{emptyString, emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"STR_SV"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING}),
+ numRows, new Object[]{emptyString});
+
+ testEmptyValues(new DataSchema(new String[]{"STR_MV"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING_ARRAY}), numRows,
+ new Object[]{emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BYTES"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BYTES}),
+ numRows, new Object[]{emptyBytes});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BOOL_ARR"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BOOLEAN_ARRAY}),
+ numRows, new Object[]{new int[]{}});
+
+ testEmptyValues(
Review Comment:
Wanna test array with 0 element and 1 element with default value.
--
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] xiangfu0 commented on a diff in pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10583:
URL: https://github.com/apache/pinot/pull/10583#discussion_r1161243653
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -123,36 +123,142 @@ public void testException(int dataTableVersion)
}
@Test(dataProvider = "versionProvider")
- public void testEmptyStrings(int dataTableVersion)
+ public void testEmptyValues(int dataTableVersion)
throws IOException {
DataTableBuilderFactory.setDataTableVersion(dataTableVersion);
String emptyString = StringUtils.EMPTY;
String[] emptyStringArray = {StringUtils.EMPTY};
+ ByteArray emptyBytes = new ByteArray(new byte[0]);
+ for (int numRows = 0; numRows < NUM_ROWS; numRows++) {
+ testEmptyValues(new DataSchema(new String[]{"STR_SV", "STR_MV"}, new DataSchema.ColumnDataType[]{
+ DataSchema.ColumnDataType.STRING, DataSchema.ColumnDataType.STRING_ARRAY
+ }), numRows, new Object[]{emptyString, emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"STR_SV"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING}),
+ numRows, new Object[]{emptyString});
+
+ testEmptyValues(new DataSchema(new String[]{"STR_MV"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING_ARRAY}), numRows,
+ new Object[]{emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BYTES"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BYTES}),
+ numRows, new Object[]{emptyBytes});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BOOL_ARR"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BOOLEAN_ARRAY}),
+ numRows, new Object[]{new int[]{}});
+
+ testEmptyValues(
Review Comment:
this should be `new int[]{0}` for boolean default
--
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] codecov-commenter commented on pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10583:
URL: https://github.com/apache/pinot/pull/10583#issuecomment-1500972985
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10583?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10583](https://codecov.io/gh/apache/pinot/pull/10583?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0e75985) into [master](https://codecov.io/gh/apache/pinot/commit/6283ee7fb8e583166966f4f5a520310f135c50d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6283ee7) will **decrease** coverage by `56.51%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10583 +/- ##
=============================================
- Coverage 70.40% 13.89% -56.51%
+ Complexity 6484 439 -6045
=============================================
Files 2106 2052 -54
Lines 112882 110393 -2489
Branches 17000 16704 -296
=============================================
- Hits 79477 15342 -64135
- Misses 27835 93798 +65963
+ Partials 5570 1253 -4317
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.89% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10583?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...apache/pinot/common/datatable/DataTableImplV3.java](https://codecov.io/gh/apache/pinot/pull/10583?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YXRhYmxlL0RhdGFUYWJsZUltcGxWMy5qYXZh) | `0.00% <0.00%> (-91.77%)` | :arrow_down: |
... and [1664 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10583/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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] xiangfu0 merged pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #10583:
URL: https://github.com/apache/pinot/pull/10583
--
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] xiangfu0 commented on pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #10583:
URL: https://github.com/apache/pinot/pull/10583#issuecomment-1500791740
@mqliang
--
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 #10583: Fix DataTableV3 serde bug for empty array
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10583:
URL: https://github.com/apache/pinot/pull/10583#discussion_r1161121341
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -123,36 +123,142 @@ public void testException(int dataTableVersion)
}
@Test(dataProvider = "versionProvider")
- public void testEmptyStrings(int dataTableVersion)
+ public void testEmptyValues(int dataTableVersion)
throws IOException {
DataTableBuilderFactory.setDataTableVersion(dataTableVersion);
String emptyString = StringUtils.EMPTY;
String[] emptyStringArray = {StringUtils.EMPTY};
+ ByteArray emptyBytes = new ByteArray(new byte[0]);
+ for (int numRows = 0; numRows < NUM_ROWS; numRows++) {
+ testEmptyValues(new DataSchema(new String[]{"STR_SV", "STR_MV"}, new DataSchema.ColumnDataType[]{
Review Comment:
i dont know if this is possible but lets also test empty `OBJECT` type?
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -123,36 +123,142 @@ public void testException(int dataTableVersion)
}
@Test(dataProvider = "versionProvider")
- public void testEmptyStrings(int dataTableVersion)
+ public void testEmptyValues(int dataTableVersion)
throws IOException {
DataTableBuilderFactory.setDataTableVersion(dataTableVersion);
String emptyString = StringUtils.EMPTY;
String[] emptyStringArray = {StringUtils.EMPTY};
+ ByteArray emptyBytes = new ByteArray(new byte[0]);
+ for (int numRows = 0; numRows < NUM_ROWS; numRows++) {
+ testEmptyValues(new DataSchema(new String[]{"STR_SV", "STR_MV"}, new DataSchema.ColumnDataType[]{
+ DataSchema.ColumnDataType.STRING, DataSchema.ColumnDataType.STRING_ARRAY
+ }), numRows, new Object[]{emptyString, emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"STR_SV"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING}),
+ numRows, new Object[]{emptyString});
+
+ testEmptyValues(new DataSchema(new String[]{"STR_MV"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.STRING_ARRAY}), numRows,
+ new Object[]{emptyStringArray});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BYTES"}, new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BYTES}),
+ numRows, new Object[]{emptyBytes});
+
+ testEmptyValues(
+ new DataSchema(new String[]{"BOOL_ARR"},
+ new DataSchema.ColumnDataType[]{DataSchema.ColumnDataType.BOOLEAN_ARRAY}),
+ numRows, new Object[]{new int[]{}});
+
+ testEmptyValues(
Review Comment:
why is this one initialized with a `{1}`?
--
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] xiangfu0 commented on pull request #10583: Fix DataTableV3 serde bug for empty array
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #10583:
URL: https://github.com/apache/pinot/pull/10583#issuecomment-1502357690
> Seems we handle `null` value everywhere, and the only place not handled is `BaseDataTable` constructor. Should we fix that instead?
I will merge this one for now. Will let @61yao to decide if the `ByteBuffer` based constructor should be moved to `BaseDataTable`.
--
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