You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "61yao (via GitHub)" <gi...@apache.org> on 2023/03/03 21:17:49 UTC
[GitHub] [pinot] 61yao opened a new pull request, #10376: [feature] [null support] selection only literal in broker null support
61yao opened a new pull request, #10376:
URL: https://github.com/apache/pinot/pull/10376
Support null literal in broker logic for selection only query.
Scalar functions are categorized as null tolerant vs intolerant.
intolerant scalar functions return null when any of the arg is null.
Introduce null columnDataType as passing used to pass broker response back to user.
--
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] mayankshriv merged pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "mayankshriv (via GitHub)" <gi...@apache.org>.
mayankshriv merged PR #10376:
URL: https://github.com/apache/pinot/pull/10376
--
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] KKcorps commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1140044860
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -230,6 +231,9 @@ private static String getStringValue(Expression thriftExpression) {
throw new BadQueryRequestException(
"Pinot does not support column or function on the right-hand side of the predicate");
}
+ if (thriftExpression.getLiteral().getSetField() == Literal._Fields.NULL_VALUE) {
+ return String.valueOf(null);
Review Comment:
We can avoid branching here and simply return "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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1148152314
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -743,6 +746,10 @@ private void verifyDataIsSame(DataTable newDataTable, DataSchema.ColumnDataType[
Assert.assertEquals(ObjectSerDeUtils.deserialize(customObject), OBJECTS[rowId], ERROR_MESSAGE);
}
break;
+ case UNKNOWN:
Review Comment:
done
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -743,6 +746,10 @@ private void verifyDataIsSame(DataTable newDataTable, DataSchema.ColumnDataType[
Assert.assertEquals(ObjectSerDeUtils.deserialize(customObject), OBJECTS[rowId], ERROR_MESSAGE);
}
break;
+ case UNKNOWN:
+ Object nulValue = newDataTable.getCustomObject(rowId, colId);
+ Assert.assertNull(nulValue, ERROR_MESSAGE);
Review Comment:
done
--
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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1148152249
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -173,7 +175,16 @@ public static Expression getLiteralExpression(byte[] value) {
return expression;
}
+ public static Expression getUnknownLiteralExpression() {
Review Comment:
done
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/CompileTimeFunctionsInvoker.java:
##########
@@ -74,7 +75,12 @@ protected static Expression invokeCompileTimeFunctionExpression(@Nullable Expres
if (functionInfo != null) {
Object[] arguments = new Object[numOperands];
for (int i = 0; i < numOperands; i++) {
- arguments[i] = function.getOperands().get(i).getLiteral().getFieldValue();
+ Literal literal = function.getOperands().get(i).getLiteral();
+ if (literal.getSetField() == Literal._Fields.NULL_VALUE) {
Review Comment:
done
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -614,6 +614,9 @@ private void fillDataTableWithRandomData(DataTableBuilder dataTableBuilder,
OBJECTS[rowId] = isNull ? null : RANDOM.nextDouble();
dataTableBuilder.setColumn(colId, OBJECTS[rowId]);
break;
+ case UNKNOWN:
Review Comment:
done
--
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] gortiz commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1144686173
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -393,6 +393,7 @@ public enum DataType {
BOOLEAN(INT, false, true),
TIMESTAMP(LONG, false, true),
STRING(false, true),
+ UNKNOWN(false, true),
Review Comment:
My point is that this PR does add `UNKNOWN` literal into `FieldSpec.DataType`, but doesn't changes any of the places where `FieldSpec.DataType` was used. This is specially important on switches. If they include a `default` case, the new literal will fall on that case, which will usually throw exceptions. In case of `switch`s without default case it can even be worse.
--
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] zhtaoxiang commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1128819056
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -154,6 +154,9 @@ public static RowDataBlock buildFromRows(List<Object[]> rows, DataSchema dataSch
case OBJECT:
setColumn(rowBuilder, byteBuffer, value);
break;
+ case UNKNOWN:
+ setColumn(rowBuilder, byteBuffer, (Object) value);
Review Comment:
(Object) value -> (Object) 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] codecov-commenter commented on pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10376:
URL: https://github.com/apache/pinot/pull/10376#issuecomment-1454178124
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10376?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 [#10376](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f32ac6) into [master](https://codecov.io/gh/apache/pinot/commit/1fce07ed77ff31af6d3b85a6b03a4b76632ed133?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1fce07e) will **decrease** coverage by `36.80%`.
> The diff coverage is `21.42%`.
```diff
@@ Coverage Diff @@
## master #10376 +/- ##
=============================================
- Coverage 68.82% 32.03% -36.80%
+ Complexity 5851 245 -5606
=============================================
Files 2027 2034 +7
Lines 109891 110276 +385
Branches 16685 16767 +82
=============================================
- Hits 75637 35327 -40310
- Misses 28897 71793 +42896
+ Partials 5357 3156 -2201
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.48% <21.42%> (?)` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.74% <0.00%> (-0.04%)` | :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/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `70.02% <0.00%> (+0.32%)` | :arrow_up: |
| [...ot/common/request/context/RequestContextUtils.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L1JlcXVlc3RDb250ZXh0VXRpbHMuamF2YQ==) | `40.57% <0.00%> (-25.33%)` | :arrow_down: |
| [...pache/pinot/common/utils/request/RequestUtils.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | `63.47% <0.00%> (-17.08%)` | :arrow_down: |
| [.../pinot/core/common/datablock/DataBlockBuilder.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9jb21tb24vZGF0YWJsb2NrL0RhdGFCbG9ja0J1aWxkZXIuamF2YQ==) | `15.65% <0.00%> (-61.08%)` | :arrow_down: |
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-82.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/utils/DataSchema.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvRGF0YVNjaGVtYS5qYXZh) | `54.23% <33.33%> (-26.71%)` | :arrow_down: |
| [.../parsers/rewriter/CompileTimeFunctionsInvoker.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9Db21waWxlVGltZUZ1bmN0aW9uc0ludm9rZXIuamF2YQ==) | `95.55% <50.00%> (-4.45%)` | :arrow_down: |
| [.../apache/pinot/common/function/FunctionInvoker.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbnZva2VyLmphdmE=) | `72.91% <60.00%> (-17.79%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/10376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1363 more](https://codecov.io/gh/apache/pinot/pull/10376?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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1147011667
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -393,6 +393,7 @@ public enum DataType {
BOOLEAN(INT, false, true),
TIMESTAMP(LONG, false, true),
STRING(false, true),
+ UNKNOWN(false, true),
Review Comment:
Oh. I''ll add the handling later since null literal isn't supported today anyway
--
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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1129932129
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -154,6 +154,9 @@ public static RowDataBlock buildFromRows(List<Object[]> rows, DataSchema dataSch
case OBJECT:
setColumn(rowBuilder, byteBuffer, value);
break;
+ case UNKNOWN:
+ setColumn(rowBuilder, byteBuffer, (Object) value);
Review Comment:
The value should already be null here. but yeah, it is more clear we pas in a null. updated the code.
--
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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1144066886
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -393,6 +393,7 @@ public enum DataType {
BOOLEAN(INT, false, true),
TIMESTAMP(LONG, false, true),
STRING(false, true),
+ UNKNOWN(false, true),
Review Comment:
Yeah. this PR just adds the interface needed and handles the broker logic.
Adding an extra type makes sure the code will error out when there is an error. instead of using some existing type to hide the error.
--
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] gortiz commented on pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10376:
URL: https://github.com/apache/pinot/pull/10376#issuecomment-1477582699
Can some contributor approve the workflow please? Otherwise we cannot see the result of the 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] gortiz commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1143152828
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -393,6 +393,7 @@ public enum DataType {
BOOLEAN(INT, false, true),
TIMESTAMP(LONG, false, true),
STRING(false, true),
+ UNKNOWN(false, true),
Review Comment:
There are like one thousand places where FieldSpec.DataType, which is specially problematic in switch. Here the datatype I proposed in https://github.com/apache/pinot/pull/10361 may be useful, but we decided to drop it.
Have you checked the impact of this new literal in these switch? Or do you plan to do that in another PR?
--
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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1142559303
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -230,6 +231,9 @@ private static String getStringValue(Expression thriftExpression) {
throw new BadQueryRequestException(
"Pinot does not support column or function on the right-hand side of the predicate");
}
+ if (thriftExpression.getLiteral().getSetField() == Literal._Fields.NULL_VALUE) {
+ return String.valueOf(null);
Review Comment:
done
--
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] Jackie-Jiang commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1148110788
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -173,7 +175,16 @@ public static Expression getLiteralExpression(byte[] value) {
return expression;
}
+ public static Expression getUnknownLiteralExpression() {
Review Comment:
(minor)
```suggestion
public static Expression getNullLiteralExpression() {
```
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -743,6 +746,10 @@ private void verifyDataIsSame(DataTable newDataTable, DataSchema.ColumnDataType[
Assert.assertEquals(ObjectSerDeUtils.deserialize(customObject), OBJECTS[rowId], ERROR_MESSAGE);
}
break;
+ case UNKNOWN:
+ Object nulValue = newDataTable.getCustomObject(rowId, colId);
+ Assert.assertNull(nulValue, ERROR_MESSAGE);
Review Comment:
(nit)
```suggestion
Assert.assertNull(newDataTable.getCustomObject(rowId, colId), ERROR_MESSAGE);
```
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/CompileTimeFunctionsInvoker.java:
##########
@@ -74,7 +75,12 @@ protected static Expression invokeCompileTimeFunctionExpression(@Nullable Expres
if (functionInfo != null) {
Object[] arguments = new Object[numOperands];
for (int i = 0; i < numOperands; i++) {
- arguments[i] = function.getOperands().get(i).getLiteral().getFieldValue();
+ Literal literal = function.getOperands().get(i).getLiteral();
+ if (literal.getSetField() == Literal._Fields.NULL_VALUE) {
Review Comment:
(minor)
```suggestion
if (literal.isSetNullValue) {
```
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -743,6 +746,10 @@ private void verifyDataIsSame(DataTable newDataTable, DataSchema.ColumnDataType[
Assert.assertEquals(ObjectSerDeUtils.deserialize(customObject), OBJECTS[rowId], ERROR_MESSAGE);
}
break;
+ case UNKNOWN:
Review Comment:
(nit) Move it to the end
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -614,6 +614,9 @@ private void fillDataTableWithRandomData(DataTableBuilder dataTableBuilder,
OBJECTS[rowId] = isNull ? null : RANDOM.nextDouble();
dataTableBuilder.setColumn(colId, OBJECTS[rowId]);
break;
+ case UNKNOWN:
Review Comment:
(nit) Move it to the end
--
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] zhtaoxiang commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1128788746
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java:
##########
@@ -32,12 +32,17 @@
*/
public class FunctionInvoker {
private final Method _method;
+ // If false, the function should return null if any of its argument is null
Review Comment:
If `false` -> if `true`?
--
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] 61yao commented on a diff in pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10376:
URL: https://github.com/apache/pinot/pull/10376#discussion_r1129931686
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java:
##########
@@ -32,12 +32,17 @@
*/
public class FunctionInvoker {
private final Method _method;
+ // If false, the function should return null if any of its argument is null
Review Comment:
Good catch. Updated the 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] gortiz commented on pull request #10376: [feature] [null support # 1] selection only literal in broker null support
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #10376:
URL: https://github.com/apache/pinot/pull/10376#issuecomment-1477582356
Can some contributor approve the workflow please? Otherwise we cannot see the result of the 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