Explicit null handling by gortiz · Pull Request #11960 · apache/pinot

gortiz · 2023-11-07T11:28:25Z

This is another approach to implement column level nullability, as explained in #10381.

Introducing null handling modes

After discussing with @Jackie-Jiang, @walterddr and @npawar we decided to propose this third approach where each table specify how to handle nulls. There are two possible ways to handle null:

Table mode, which is what we were using so far
Column mode, which means that each column specifies its own nullability in the FieldSpec.

Multi-stage engine (aka V2) has also been modified to reject queries where SET enableNullHandling = true AND at least one of the tables involved in the query uses Table mode null handling.

TODO

Modifications in Schema
Modifications in TypeFactory
Modifications in NullValueIndexType
Modifications in the ingestion side
Modifications in test engine
Write resource tests

Proposed Syntax

Although the null handling mode semantics are clear, it is not so clear how to configure them. What is explained here is just a proposal open to discussion and can be changed during the PR review.

The proposal is to add a new JSON property in the schema*.

This new property can be configured as:

{
  "schemaName": "blablabla",
  "dimensionFieldSpecs": [
    {
      "dataType": "INT",
      "name": "nullableField",
      "notNull": false
    },
    {
      "dataType": "INT",
      "name": "notNullableField",
      "notNull": true
    },
    {
      "dataType": "INT",
      "name": "defaultNullableField"
    },
    ...
  ],
  "enableColumnBasedNullHandling": true/false
}

The default value for enableColumnBasedNullHandling is false, which keeps compatibility with previous code. When it is changed to true, Pinot will ignore TableConfig.IndexingConfig.nullHandlingEnabled and columns will be nullable if and only if FieldSpec.notNull is false, which is also the default value.

* It is yet to be decide whether it will be added to the TableConfig or the Schema. When this PR was open it was added to the Schema.

In production this should have no impact, but will keep current behavior when ImmutableSegment.load(File, ReadMode) is called.

…Value is specified.

…ng them

…ified

This is needed to keep backward compatibility with previous tests.

…testNG parallelism.

gortiz · 2023-11-16T14:34:00Z

In https://github.com/apache/pinot/actions/runs/6874613568/job/18696611890?pr=11960 you can see the test that failed. I'm going to comment them again in order to have a green light.

walterddr · 2023-11-16T15:59:15Z

In https://github.com/apache/pinot/actions/runs/6874613568/job/18696611890?pr=11960 you can see the test that failed. I'm going to comment them again in order to have a green light.

the output value was changed in #11234 CC @Jackie-Jiang to take a look.

walterddr

looks good to me mostly. please kindly address the follow ups. mostly on changes that can be separate into different PR

walterddr · 2023-11-17T17:26:58Z

...y-runtime/src/test/java/org/apache/pinot/query/runtime/queries/ResourceBasedQueriesTest.java

  private static final String FILE_FILTER_PROPERTY = "pinot.fileFilter";
  private static final String IGNORE_FILTER_PROPERTY = "pinot.runIgnored";
  private static final int DEFAULT_NUM_PARTITIONS = 4;
+  private static final boolean REPORT_IGNORES = true;


can we separate this one from this PR?

I've rollbacked most of these changes in 3dd0886

...-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTestBase.java

walterddr · 2023-11-17T17:30:39Z

...-query-planner/src/test/java/org/apache/pinot/query/queries/ResourceBasedQueryPlansTest.java

  }

-  @DataProvider
+  @DataProvider(parallel = true)


i wasn't sure this is fully verified. if possible let's keep this separate into another PR

...src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java

.../apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java

…al/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

…s false and nullHandlingEnabled is true

…chema-null-handling

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2023-11-17T18:09:00Z

pinot-integration-tests/src/test/resources/test_null_handling.schema

+  "options": {
+    "nullHandling": {
+      "mode": "column",
+      "default": true


^^
Also, how does it work right now? Do we have test that rely on column based null handling to be enabled?

...-query-planner/src/test/java/org/apache/pinot/query/testutils/MockRoutingManagerFactory.java

Jackie-Jiang · 2023-11-17T18:11:15Z

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

      {
        "comments": "table aren't actually partitioned by val thus all segments can produce duplicate results, thus [[8]]",
        "sql": "SELECT SEGMENT_PARTITIONED_DISTINCT_COUNT(val) FROM {tbl1}",
+        "ignored": true,


Shall we try removing the ignored flag and see if they still fail? If so, we might want to debug them because this can cause regression

walterddr · 2023-11-20T16:57:33Z

...test/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexTypeTest.java


    @Test(dataProvider = "provideCases", dataProviderClass = NullValueIndexTypeTest.class)
    public void isEnabledWhenNullable(boolean columnNullHandling, boolean fieldNullable, IndexConfig expected) {
+      _tableConfig.getIndexingConfig().setNullHandlingEnabled(true);


this is needed b/c the logic now rely on table config. (backward incompat?)

gortiz added 20 commits October 25, 2023 10:41

Add nullability to FieldSpec and use it in TypeFactory

e5d9754

Change NullValueIndexType to skip not nullable columns

b7cc9aa

Enable NullValueIndexType by default in order to pass tests.

7f52f65

In production this should have no impact, but will keep current behavior when ImmutableSegment.load(File, ReadMode) is called.

Remove the assumption of being nullable for columns where defaultNull…

dcf1e33

…Value is specified.

Remove unnecessary changes in TypeSystem

9d80da3

Prepare test framework to add more tests

6d02a0e

Remove space

97660bb

Actually ignore tests cases marked as ingored instead of not generati…

64ea0cd

…ng them

Add some null tests

6a159ef

Ignore some tests that started to fail once nullHandling.json was mod…

c8105aa

…ified

Fix false errors in test

675601a

Honor EnableNullHandling in tests

8f108be

Add some extra tests

9ae4edb

Change nullable semantic to relay only on FieldSpec

63146e7

Adapt NullValueIndexTypeTest to not nullable FieldSpec.isNullable()

e918c43

Adapt TypeFactoryTest to nullable by default

6843a42

Fix NullValueIndexTypeTest

698573c

Change QueryEnvironmentTestBase to be not nullable.

fea29b8

This is needed to keep backward compatibility with previous tests.

Small change to make ResourceBasedQueryPlansTest run faster by using …

607c764

…testNG parallelism.

Add the concept of null handling at schema level

e1a2e49

gortiz marked this pull request as draft November 7, 2023 11:28

gortiz added 9 commits November 7, 2023 13:03

Move Nullhandlig to its own class

3352c10

Modify SegmentColumnarIndexCreator to support NullHandling

6155ab6

Add license header

a1eebdf

Add javadoc on NullHandling.supportsV2

c77c480

Add TODO

9d48652

Adapt new columns empty columns to NullHandling

242c35e

Improve error message when no table config is present

2df1298

Make Schema.Options and NullHandling Serializables

a9ba1e4

Add options to special serialization method of Schema

7d6adc2

gortiz added 2 commits November 16, 2023 15:37

Ignore test incorrect tests in CountDistinct.json

17ec649

Report ignored tests again

9e30389

walterddr approved these changes Nov 17, 2023

View reviewed changes

gortiz added 3 commits November 17, 2023 18:43

Remove parallel testng modifier

e73aac4

Remove test improvements not specific to this PR

3dd0886

Fix style

7716fe0

Jackie-Jiang reviewed Nov 17, 2023

View reviewed changes

...src/main/java/org/apache/pinot/segment/local/segment/index/nullvalue/NullValueIndexType.java Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Nov 17, 2023

View reviewed changes

.../apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java Outdated Show resolved Hide resolved

gortiz and others added 4 commits November 17, 2023 18:54

Remove sql from assertions

882aafd

Update pinot-segment-local/src/main/java/org/apache/pinot/segment/loc…

9c2d47f

…al/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

Consider null index enabled also when enableColumnBasedNullHandling i…

9402a7c

…s false and nullHandlingEnabled is true

Merge branch 'schema-null-handling' of github.com:gortiz/pinot into s…

3a5e3f6

…chema-null-handling

Jackie-Jiang approved these changes Nov 17, 2023

View reviewed changes

fix test

278c11a

walterddr reviewed Nov 20, 2023

View reviewed changes

Jackie-Jiang added 2 commits November 20, 2023 14:15

Fix Mutable segment null handling, test, and minor cleanup

05234fe

Fix lint

c85b991

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes Configuration Config changes (addition/deletion/change in behavior) multi-stage Related to the multi-stage query engine documentation labels Nov 20, 2023

Jackie-Jiang merged commit 35748a0 into apache:master Nov 20, 2023

gortiz deleted the schema-null-handling branch December 7, 2023 21:05

gortiz mentioned this pull request Jan 5, 2024

Add nullability to FieldSpec and use it in TypeFactory #11824

Closed

7 tasks

gortiz mentioned this pull request Aug 22, 2024

RealtimeSegmentConverter was using incorrect schema #13877

Merged

yashmayya added the null support label Aug 27, 2024

yashmayya mentioned this pull request Aug 27, 2024

Support selective nullable columns #10652

Closed

Conversation

gortiz commented Nov 7, 2023 • edited by walterddr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introducing null handling modes

TODO

Proposed Syntax

Uh oh!

gortiz commented Nov 16, 2023

Uh oh!

walterddr commented Nov 16, 2023

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

gortiz Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

gortiz Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

walterddr Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jackie-Jiang Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

walterddr Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gortiz commented Nov 7, 2023 •

edited by walterddr

Loading