[multistage] proper support for column-level null handling #10423

walterddr · 2023-03-15T04:26:33Z

This is a follow-up on #10381 and a better version of #10148

This PR:

support null enabled planning if the table supports null
creates a proper plan for null-enabled tables.
column level optimization for nullability (see flagging columns null instead of null from table config #10428)
- added a flag for nullable in FieldSpec

ankitsultana · 2023-03-15T06:56:46Z

Thanks.. this is good to see. One note: this is setting nullability at table-level.

Most of our use-cases require nullability to be set at column-level. People want do things like IS NULL on some columns while at the same time they want to use sub-queries with semi-joins (uuid IN (...)), so they want uuid to be non-null.

I think long-term we may want column level nullability, in which case we may need to think how backwards compatibility will be handled if we go with table level nullability right now.

walterddr · 2023-03-16T04:35:04Z

Thanks.. this is good to see. One note: this is setting nullability at table-level.

Most of our use-cases require nullability to be set at column-level. People want do things like IS NULL on some columns while at the same time they want to use sub-queries with semi-joins (uuid IN (...)), so they want uuid to be non-null.

I think long-term we may want column level nullability, in which case we may need to think how backwards compatibility will be handled if we go with table level nullability right now.

i think this make sense. created a separate ticket for this.

pinot-query-runtime/src/test/resources/queries/Comparisons.json

ankitsultana · 2023-03-16T08:38:59Z

Just to confirm: we are putting this on hold right?

walterddr · 2023-03-16T15:47:07Z

Just to confirm: we are putting this on hold right?

the table-level nullability handle will be checked in first and then we will enable column-level nullability.
as long as table didn't enable null handling we shouldn't have any issue.

walterddr · 2023-03-16T17:16:15Z

the table-level nullability handle will be checked in first and then we will enable column-level nullability. as long as table didn't enable null handling we shouldn't have any issue.

Chatted offline. we will hold off b/c without column level nullability this will cause large planner deficiency --> will address #10428 first

codecov-commenter · 2023-03-17T20:22:02Z

Codecov Report

❌ Patch coverage is 0% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.11%. Comparing base (2d30e28) to head (b51c016).
⚠️ Report is 3959 commits behind head on master.

Files with missing lines	Patch %	Lines
.../java/org/apache/pinot/query/type/TypeFactory.java	0.00%	20 Missing ⚠️
...a/org/apache/pinot/query/catalog/PinotCatalog.java	0.00%	11 Missing ⚠️
...main/java/org/apache/pinot/spi/data/FieldSpec.java	0.00%	11 Missing ⚠️
...ava/org/apache/pinot/query/catalog/PinotTable.java	0.00%	3 Missing ⚠️
...n/function/scalar/DataTypeConversionFunctions.java	0.00%	2 Missing ⚠️
.../pinot/common/function/scalar/ObjectFunctions.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10423      +/-   ##
==========================================
- Coverage    0.11%    0.11%   -0.01%     
==========================================
  Files        2190     2190              
  Lines      118056   118081      +25     
  Branches    17873    17878       +5     
==========================================
  Hits          137      137              
- Misses     117899   117924      +25     
  Partials       20       20

Flag	Coverage Δ
integration1temurin11	`0.00% <0.00%> (ø)`
integration1temurin17	`0.00% <0.00%> (ø)`
integration1temurin20	`0.00% <0.00%> (ø)`
integration2temurin11	`0.00% <0.00%> (ø)`
integration2temurin17	`0.00% <0.00%> (ø)`
integration2temurin20	`0.00% <0.00%> (ø)`
unittests1temurin11	`0.00% <0.00%> (ø)`
unittests1temurin17	`0.00% <0.00%> (ø)`
unittests1temurin20	`0.00% <0.00%> (ø)`
unittests2temurin11	`0.11% <0.00%> (-0.01%)`	⬇️
unittests2temurin17	`0.11% <0.00%> (-0.01%)`	⬇️
unittests2temurin20	`0.11% <0.00%> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

walterddr · 2023-03-22T16:53:32Z

this PR is ready for review CC @ankitsultana @Jackie-Jiang

ankitsultana · 2023-03-29T18:31:46Z

pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java

Should this be "and"? isNullSupportEnabled && e.getValue().isNullableField()

Since fieldSpec is already passed to toRelDataType, we can also pass only isNullSupportEnabled here and in toRelDataType we can do:

return createTypeWithNullability(fieldSpec.isSingleValueField() ? createSqlType(SqlTypeName.INTEGER) : createArrayType(createSqlType(SqlTypeName.INTEGER), -1), isNullSupportEnabled && fieldSpec.isNullableField());

no and to answer the previous comment as well.

we can either enable globally (using table config), or enable via column config

if global enable. then all fields are nullable.

if not global enable. we read column level nullability.

if neither is set, then the column is not nullable

What about a fallback mechanism to determine nullability?

A column level nullability can be true, false, or NULL(default).

A table level nullability can be true or false (default)

If the column level nullability is present, it determines the nullability.

If the column level nullability is not present, we fall back to the table level nullability setting.

Or the table level nullability can also be NULL (default). If both the column level nullability and table level nullability are NULL, we apply the current behavior.

I was going to propose the same @shenyu0127 proposed in June

pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotTable.java

ankitsultana · 2023-03-29T20:01:03Z

pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java

Does this need to be: nullHandlingMap.getOrDefault(entry.getKey(), true)?

no. by default nullability is disabled. this is the default behavior of all tables for pinot nowadays.

The comment was wrt // ALWAYS enable null handling in V2.

ah i think the comment was not suppose to be there. what i meant was "nullhandling query option" was enabled always, not "null column is always registered for table" :-P

ankitsultana · 2023-03-29T20:04:27Z

pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java

We need to get TableConfig for at least one type in a bunch of places in the code. Can we add a util method somewhere?

good idea but let's follow up?

ankitsultana · 2023-03-29T20:07:41Z

pinot-query-runtime/src/test/resources/queries/NullHandling.json

Can you also add some plan-based tests in pinot-query-planner?

Particularly for anti semi-join queries such as: SELECT .. FROM T1 WHERE uuid NOT IN (SELECT uuid FROM ...)

If the uuid is column is nullable, this gets converted to a broadcast join (iirc).

Would be great if we can add test-cases for these.

added planner test

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

ankitsultana · 2023-04-04T05:40:00Z

@walterddr : This query is one example where the join changes to a broadcast join after this change (you'll need to set nullHandlingEnabled: true in TableConfig for both tables).

explain plan for select count(*), deviceOS from userAttributes_OFFLINE 
  WHERE userUUID NOT IN (SELECT userUUID FROM userGroups_OFFLINE WHERE groupUUID = 'group-1')
group by deviceOS

Edit: Also this variant of the query also does a broadcast join:

explain plan for select count(*), deviceOS from userAttributes_OFFLINE 
  WHERE NOT (userUUID IN (SELECT userUUID FROM userGroups_OFFLINE WHERE groupUUID = 'group-1'))
group by deviceOS

ankitsultana · 2023-04-04T05:41:25Z

pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java

^ Let's add a table or two which have nullHandlingEnabled: true in table-config. Anti semi join queries (NOT IN) kind of queries would turn into broadcast joins in that case.

walterddr · 2023-04-26T14:58:37Z

any additional comment on this approach? @ankitsultana @61yao @Jackie-Jiang

ankitsultana · 2023-05-03T04:13:46Z

My main concern is with this part:

builder.add(e.getKey(), toRelDataType(e.getValue(),
          isNullSupportEnabled || e.getValue().isNullableField()));

I think if users have configured "enableNullHandling" at table level, we should let column level nullability decide whether a given column is nullable or not.

If enableNullHandling is not set at the table level, then we can assume that all columns are non-null (similar to V1 engine). That would essentially mean that column level nullability is not enabled so we assume everything to be non-null.

walterddr · 2023-05-03T04:40:38Z

pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java

wouldn't this address your concern @ankitsultana

walterddr · 2023-05-03T04:43:46Z

My main concern is with this part:
builder.add(e.getKey(), toRelDataType(e.getValue(),
          isNullSupportEnabled || e.getValue().isNullableField()));
I think if users have configured "enableNullHandling" at table level, we should let column level nullability decide whether a given column is nullable or not.

If enableNullHandling is not set at the table level, then we can assume that all columns are non-null (similar to V1 engine). That would essentially mean that column level nullability is not enabled so we assume everything to be non-null.

Hmm. I understood your concern. so to link: https://github.com/apache/pinot/pull/10423/files#r1183232264 --> this means in order to enable nullability we should

only do so for a table if table config enables null, otherwise disregard any column level config (this is existing behavior)
add column level nullability config to user setting it to false, otherwise default to true (this is going to change)
create nullable column only if (1) AND (2) are true

yes?

ankitsultana · 2023-05-03T15:33:53Z

My main concern is with this part:
builder.add(e.getKey(), toRelDataType(e.getValue(),
          isNullSupportEnabled || e.getValue().isNullableField()));
I think if users have configured "enableNullHandling" at table level, we should let column level nullability decide whether a given column is nullable or not.
If enableNullHandling is not set at the table level, then we can assume that all columns are non-null (similar to V1 engine). That would essentially mean that column level nullability is not enabled so we assume everything to be non-null.
Hmm. I understood your concern. so to link: https://github.com/apache/pinot/pull/10423/files#r1183232264 --> this means in order to enable nullability we should

only do so for a table if table config enables null, otherwise disregard any column level config (this is existing behavior)

add column level nullability config to user setting it to false, otherwise default to true (this is going to change)

create nullable column only if (1) AND (2) are true

yes?

Yeah. The idea being that we only have two states:

Either column level nullability feature is disabled (when enableNullHandling is not set at table level)
Or it is enabled, in which case its values will always be honored.

- index on planner changed - test example in runtime changed

Jackie-Jiang

Suggest making a separate PR for the FieldSpec and v1 related changes to limit the scope so that it is easier to review. We want to handle backward compatibility for table level enableNullHandling

Jackie-Jiang · 2023-06-29T00:04:38Z

...ommon/src/main/java/org/apache/pinot/common/function/scalar/DataTypeConversionFunctions.java


  @ScalarFunction
  public static Object cast(Object value, String targetTypeLiteral) {
+    if (value == null) {


We shouldn't need to handle null here if the scalar function doesn't have nullableParameters

shenyu0127 · 2023-06-29T19:22:15Z

pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeFactory.java

What about a fallback mechanism to determine nullability?

A column level nullability can be true, false, or NULL(default).

A table level nullability can be true or false (default)

If the column level nullability is present, it determines the nullability.

If the column level nullability is not present, we fall back to the table level nullability setting.

shenyu0127 · 2023-06-29T20:20:31Z

pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java

    return this;
  }
+
+  private static boolean isNullEnabled(TableCache tableCache, String rawTableName) {


This function gets a table property. Other similar functions are member functions. Do we want change this function to a member function for consistency?

walterddr · 2023-09-28T20:42:23Z

closing this one and creating a new one (design doc shared in #10381)

walterddr force-pushed the pr_fix_null_handling branch 2 times, most recently from 1e7b49a to 1c364a5 Compare March 16, 2023 00:46

walterddr commented Mar 16, 2023

View reviewed changes

pinot-query-runtime/src/test/resources/queries/Comparisons.json Outdated Show resolved Hide resolved

walterddr force-pushed the pr_fix_null_handling branch from 684a58f to 522228d Compare March 22, 2023 14:52

walterddr requested review from Jackie-Jiang and xiangfu0 March 22, 2023 16:53

walterddr force-pushed the pr_fix_null_handling branch from 522228d to e306753 Compare March 23, 2023 14:40

walterddr changed the title ~~[multistage] proper support for null handling~~ [multistage] proper support for column-level null handling Mar 29, 2023

walterddr force-pushed the pr_fix_null_handling branch from e306753 to 97ece6d Compare March 29, 2023 17:55

ankitsultana reviewed Mar 29, 2023

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java Outdated Show resolved Hide resolved

ankitsultana reviewed Apr 4, 2023

View reviewed changes

walterddr force-pushed the pr_fix_null_handling branch from 9890bf5 to f6708ab Compare April 6, 2023 17:47

abhioncbr mentioned this pull request Apr 11, 2023

[WIP]: 10513 - Null support in Case/When queries. #10514

Closed

walterddr mentioned this pull request Apr 17, 2023

[multistage] populate queryOption down to leaf #10626

Merged

walterddr force-pushed the pr_fix_null_handling branch from f6708ab to c7878db Compare April 19, 2023 16:52

Jackie-Jiang added feature multi-stage Related to the multi-stage query engine labels Apr 22, 2023

walterddr commented May 3, 2023

View reviewed changes

null handling

29324d7

walterddr force-pushed the pr_fix_null_handling branch from 2ddcc8a to 29324d7 Compare June 27, 2023 03:08

fix tests

3b6ef89

- index on planner changed - test example in runtime changed

walterddr force-pushed the pr_fix_null_handling branch from 62832cc to 3b6ef89 Compare June 27, 2023 18:58

fix some rebase merge issues

b51c016

Jackie-Jiang reviewed Jun 29, 2023

View reviewed changes

shenyu0127 reviewed Jun 29, 2023

View reviewed changes

walterddr mentioned this pull request Jul 21, 2023

[multistage][agg] support agg with filter #11144

Merged

walterddr mentioned this pull request Sep 27, 2023

[multistage] IS NULL/ IS NOT NULL getting skipped from the queries using multistage #11691

Closed

walterddr closed this Sep 28, 2023

walterddr mentioned this pull request Sep 28, 2023

[multistage] Null Check Predicates Not Pushed Down #10381

Closed

[multistage] proper support for column-level null handling #10423

[multistage] proper support for column-level null handling #10423

Uh oh!

Conversation

walterddr commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitsultana commented Mar 15, 2023

Uh oh!

walterddr commented Mar 16, 2023

Uh oh!

Uh oh!

ankitsultana commented Mar 16, 2023

Uh oh!

walterddr commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walterddr commented Mar 16, 2023

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

walterddr commented Mar 22, 2023

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!

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!

walterddr Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

ankitsultana commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr commented Apr 26, 2023

Uh oh!

ankitsultana commented May 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterddr commented May 3, 2023

Uh oh!

ankitsultana commented May 3, 2023

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!

walterddr commented Sep 28, 2023

walterddr commented Mar 15, 2023 •

edited

Loading

walterddr commented Mar 16, 2023 •

edited

Loading

codecov-commenter commented Mar 17, 2023 •

edited

Loading

walterddr Mar 29, 2023 •

edited

Loading

ankitsultana commented Apr 4, 2023 •

edited

Loading