[feature] [null support # 1] selection only literal in broker null support #10376

61yao · 2023-03-03T21:17:47Z

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.

codecov-commenter · 2023-03-03T21:58:41Z

Codecov Report

Merging #10376 (9a8ace5) into master (8bfb0b8) will increase coverage by 50.12%.
The diff coverage is 46.66%.

@@              Coverage Diff              @@
##             master   #10376       +/-   ##
=============================================
+ Coverage     13.97%   64.09%   +50.12%     
- Complexity      237     6090     +5853     
=============================================
  Files          2016     2016               
  Lines        109575   109491       -84     
  Branches      16778    16759       -19     
=============================================
+ Hits          15315    70181    +54866     
+ Misses        93012    34172    -58840     
- Partials       1248     5138     +3890

Flag	Coverage Δ
unittests1	`67.87% <51.85%> (?)`
unittests2	`13.98% <0.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...roker/requesthandler/BaseBrokerRequestHandler.java	`46.62% <0.00%> (-0.16%)`	⬇️
...ot/common/request/context/RequestContextUtils.java	`65.14% <0.00%> (+65.14%)`	⬆️
...pache/pinot/common/utils/request/RequestUtils.java	`67.52% <0.00%> (+67.52%)`	⬆️
.../parsers/rewriter/CompileTimeFunctionsInvoker.java	`95.55% <50.00%> (+95.55%)`	⬆️
.../apache/pinot/common/function/FunctionInvoker.java	`87.50% <60.00%> (+87.50%)`	⬆️
...java/org/apache/pinot/common/utils/DataSchema.java	`76.89% <100.00%> (+76.89%)`	⬆️
.../pinot/core/common/datablock/DataBlockBuilder.java	`77.86% <100.00%> (+77.86%)`	⬆️
...main/java/org/apache/pinot/spi/data/FieldSpec.java	`82.07% <100.00%> (+82.07%)`	⬆️

... and 1348 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zhtaoxiang · 2023-03-08T00:24:50Z

pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java

If false -> if true?

Good catch. Updated the comment.

zhtaoxiang · 2023-03-08T01:03:02Z

pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java

(Object) value -> (Object) null?

The value should already be null here. but yeah, it is more clear we pas in a null. updated the code.

KKcorps · 2023-03-17T10:18:41Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java

We can avoid branching here and simply return "null"

gortiz · 2023-03-21T10:15:11Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java

There are like one thousand places where FieldSpec.DataType, which is specially problematic in switch. Here the datatype I proposed in #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?

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.

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 switchs without default case it can even be worse.

Oh. I''ll add the handling later since null literal isn't supported today anyway

gortiz · 2023-03-21T10:18:42Z

Can some contributor approve the workflow please? Otherwise we cannot see the result of the tests.

gortiz

LGTM

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2023-03-24T22:24:33Z

pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java

    return expression;
  }

+  public static Expression getUnknownLiteralExpression() {


(minor)

Suggested change

public static Expression getUnknownLiteralExpression() {

public static Expression getNullLiteralExpression() {

Jackie-Jiang · 2023-03-24T22:25:15Z

...-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/CompileTimeFunctionsInvoker.java

        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) {


(minor)

Suggested change

if (literal.getSetField() == Literal._Fields.NULL_VALUE) {

if (literal.isSetNullValue) {

Jackie-Jiang · 2023-03-24T22:26:11Z

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java

            OBJECTS[rowId] = isNull ? null : RANDOM.nextDouble();
            dataTableBuilder.setColumn(colId, OBJECTS[rowId]);
            break;
+          case UNKNOWN:


(nit) Move it to the end

Jackie-Jiang · 2023-03-24T22:26:21Z

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java

              Assert.assertEquals(ObjectSerDeUtils.deserialize(customObject), OBJECTS[rowId], ERROR_MESSAGE);
            }
            break;
+          case UNKNOWN:


(nit) Move it to the end

Jackie-Jiang · 2023-03-24T22:27:25Z

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java

+            Object nulValue = newDataTable.getCustomObject(rowId, colId);
+            Assert.assertNull(nulValue, ERROR_MESSAGE);


(nit)

Suggested change

Object nulValue = newDataTable.getCustomObject(rowId, colId);

Assert.assertNull(nulValue, ERROR_MESSAGE);

Assert.assertNull(newDataTable.getCustomObject(rowId, colId), ERROR_MESSAGE);

61yao changed the title ~~[feature] [null support] selection only literal in broker null support~~ [feature] [null support # 1] selection only literal in broker null support Mar 3, 2023

zhtaoxiang reviewed Mar 8, 2023

View reviewed changes

KKcorps reviewed Mar 17, 2023

View reviewed changes

gortiz reviewed Mar 21, 2023

View reviewed changes

gortiz mentioned this pull request Mar 21, 2023

[feature] [backward-incompat] [null support # 2] Preserve null literal information in literal context and literal transform #10380

Merged

gortiz approved these changes Mar 24, 2023

View reviewed changes

zhtaoxiang approved these changes Mar 24, 2023

View reviewed changes

KKcorps approved these changes Mar 24, 2023

View reviewed changes

Yao Liu added 4 commits March 24, 2023 14:21

selection only literal in broker null support

a63b2d3

fix test

f0de850

address comments

1cb9f6a

address comment

53b1113

61yao force-pushed the null1 branch from 7300089 to 53b1113 Compare March 24, 2023 21:21

move null to last

4d4040f

Jackie-Jiang approved these changes Mar 24, 2023

View reviewed changes

address comments

9a8ace5

mayankshriv merged commit d1227e4 into apache:master Mar 25, 2023

61yao mentioned this pull request Mar 31, 2023

[Feature] Null support in Pinot #10252

Closed

yashmayya added the null support label Sep 6, 2024

	public static Expression getUnknownLiteralExpression() {
	public static Expression getNullLiteralExpression() {

	if (literal.getSetField() == Literal._Fields.NULL_VALUE) {
	if (literal.isSetNullValue) {

		Object nulValue = newDataTable.getCustomObject(rowId, colId);
		Assert.assertNull(nulValue, ERROR_MESSAGE);

	Object nulValue = newDataTable.getCustomObject(rowId, colId);
	Assert.assertNull(nulValue, ERROR_MESSAGE);
	Assert.assertNull(newDataTable.getCustomObject(rowId, colId), ERROR_MESSAGE);

[feature] [null support # 1] selection only literal in broker null support #10376

[feature] [null support # 1] selection only literal in broker null support #10376

Uh oh!

Conversation

61yao commented Mar 3, 2023

Uh oh!

codecov-commenter commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gortiz commented Mar 21, 2023

Uh oh!

gortiz left a comment

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-commenter commented Mar 3, 2023 •

edited

Loading