Skip to content

Fix Windowing/scanAndSort query issues on top of Joins.#15996

Merged
abhishekagarwal87 merged 9 commits intoapache:masterfrom
kgyrtkirk:scanandsort-hashjoin-adapter-submit
Mar 5, 2024
Merged

Fix Windowing/scanAndSort query issues on top of Joins.#15996
abhishekagarwal87 merged 9 commits intoapache:masterfrom
kgyrtkirk:scanandsort-hashjoin-adapter-submit

Conversation

@kgyrtkirk
Copy link
Copy Markdown
Member

@kgyrtkirk kgyrtkirk commented Feb 28, 2024

  • allow a hashjoin result to be converted to RowsAndColumns
    • added StorageAdapterRowsAndColumns
  • fix incorrect isConcrete() return values during early phase of planning

Fixes #XXXX.

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note


Key changed/added classes in this PR
  • MyFoo
  • OurBar
  • TheirBaz

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

* allow a hashjoin result to be converted to RowsAndColumns
* fix incorrect isConcrete() return values during early phase of planning
@kgyrtkirk kgyrtkirk changed the title Fix windowing operator issues on top of Joins. Fix Windowing/scanAndSort query issues on top of Joins. Feb 28, 2024
@kgyrtkirk kgyrtkirk marked this pull request as ready for review February 28, 2024 21:46
Copy link
Copy Markdown
Contributor

@imply-cheddar imply-cheddar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. Given that this code path should hopefully only run on top of the results of another query, I suspect that it will be fine. I'll admit that I find StorageAdapterRowsAndColumns to be a little gross (it's like... doing something but trying not to do anything), but it should just work for the intended purpose here.

return new ColumnPlus(
frameCol,
ColumnCapabilitiesImpl.createSimpleNumericColumnCapabilities(frameCol.getType())
.setHasNulls(NullHandling.sqlCompatible() && frameCol.hasNulls),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I think even in case we are defaulting null-s to something somewhere; we may still have the null value.
    running the following query with druid-27.0.0 with druid.generic.useDefaultValueForNull: true
select null,'',0,cast(null as integer)
null |  empty | 0 | null 

which suggests that the column value can be null

  • in this case the incoming column also reported that it has nulls - I think it shouldn't be allowed to narrow the scope

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cast thing returning null is probably a bug (though its been a bug for so long that imo it isn't worth fixing in a deprecated mode), the native cast expression would spit out 0 from an expression selector if it actually ran...

I think to not risk any inadvertent changes we shouldn't make this change for now if possible. Default value mode is deprecated and will be removed in a future release, so we should probably just leave things as they are whenever possible.

Copy link
Copy Markdown
Member Author

@kgyrtkirk kgyrtkirk Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that:

  • the system do allow null -s - via by a simple cast - so null exists even in defaultValue mode - so I don't see it bad...
  • I believe this codepath is only triggered for windowoperator related stuff - but I'll check it (I'll update this comment when it finishes)
  • the incoming frame declares that it has null values (which is being covered)
  • we also have the ClusteredGroupPartitionerTest#testDefaultClusteredGroupPartitionerWithNulls which essentially inputs null-s as inputs...should I disable that testcase for defaultValues ? it will most likely just hide the issue I was facing - but I can live with that...should I just do that?

I believe the original logic incorrectly narrows to doesn't have nulls (NullHandling.sqlCompatible() && frameCol.hasNulls) - which could be false irrsepectively that the backing one does have (frameCol.hasNulls) ...if that's not allowed an exception should be thrown right away and not take further risks...

I see the following options:

  • allow nulls - as the incoming frame already contained them
  • raise an exception and mark the testcase ClusteredGroupPartitionerTest#testDefaultClusteredGroupPartitionerWithNulls not apply when useDefaultValue is enabled

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken the second approach:

  • restored these classes to the previous state
  • added an assumeTrue(sqlCompatible) to the testcase

I still think that it might not be a good idea to retain/make tweaks like this...null does exists in the system even when defaultValue is enabled...so it may naturally happen as a value in a column as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants