Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: DH-16595: Fix null partition filter #1954

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

vbabich
Copy link
Collaborator

@vbabich vbabich commented Apr 22, 2024

Closes #1867

Tested in Enterprise using steps from DH-16595 - created a partitioned table and set ACL to nullFilterGenerator() to get the null value in the partition column. Creating a partitioned table with a null in the partition column doesn't seem possible with db.addTablePartition in Enterprise.

t=db.t("LearnDeephaven", "StockTrades").where()
db.addPartitionedTableDefinition("<namespace>", "partTable", "Date", t.getDefinition())
test = db.t("<namespace>" , "partTable")

// Set Column ACL on `partTable` to `new NullFilterGenerator()` in swing

test = db.t("<namespace>" , "partTable")

Screenshot 2024-04-23 at 5 26 43 PM

Core+ Groovy console:

t = emptyTable(1).update("X=(String)null","Y=ii").partitionBy("X")

Screenshot 2024-04-23 at 5 23 55 PM

@vbabich vbabich self-assigned this Apr 22, 2024
@vbabich vbabich requested a review from mofojed April 22, 2024 13:56
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Minor display string change, can you also include how this was tested in the PR description?

packages/iris-grid/src/IrisGridPartitionSelector.tsx Outdated Show resolved Hide resolved
@vbabich
Copy link
Collaborator Author

vbabich commented Apr 23, 2024

Changed null to (null), added testing steps to the description.

@vbabich
Copy link
Collaborator Author

vbabich commented Apr 23, 2024

I tested this in a Core+ worker and it looks like the api returns undefined for a null partition key value. Will need to investigate a bit more.

@vbabich
Copy link
Collaborator Author

vbabich commented Apr 23, 2024

Partitioned table with a null key in DHE:

Screenshot 2024-04-23 at 11 34 44 AM

DHC shows an extra empty option element until the core PR is merged.

Screenshot 2024-04-23 at 2 14 57 PM

@vbabich vbabich requested a review from mofojed April 23, 2024 20:35
@vbabich
Copy link
Collaborator Author

vbabich commented Apr 23, 2024

Added nullish coalescing to account for undefined returned by the JSAPI for null table values

mofojed
mofojed previously approved these changes Apr 24, 2024
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Minor suggestion to include the core ticket number.

packages/iris-grid/src/IrisGrid.tsx Show resolved Hide resolved
Co-authored-by: Mike Bender <mikebender@deephaven.io>
@vbabich vbabich enabled auto-merge (squash) April 24, 2024 17:18
@vbabich vbabich changed the title fix: Fix null partition filter fix: DH-16595: Fix null partition filter Apr 24, 2024
@vbabich vbabich requested a review from mofojed April 24, 2024 17:19
@vbabich vbabich merged commit 3a1f92b into deephaven:main Apr 24, 2024
5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table with a null partition value throws NPE
2 participants