-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Add support for multi-partition parquet:kv tables #1580
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1580 +/- ##
==========================================
+ Coverage 46.65% 46.73% +0.08%
==========================================
Files 575 583 +8
Lines 36189 36324 +135
Branches 9063 9095 +32
==========================================
+ Hits 16883 16977 +94
- Misses 19254 19295 +41
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
5326170
to
ac3d366
Compare
…o 1438-multi-partition
ac3d366
to
c8b5b1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to worry about legacy format from dehydrated state, so update that type and handle it correctly in hydrateIrisGridPanelState
.
9610545
to
a6d31a1
Compare
b0a89a9
to
9c9d099
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, but with the added hydration method and the imperative that we handle legacy data correctly, we need to add unit tests that cover those hydration cases.
0548e5d
to
4875616
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests need a bit of cleanup
'hydrateIrisGridPanelStateV1 unselected partition', | ||
{ | ||
isSelectingPartition: false, | ||
partition: '', | ||
partitionColumn: 'name_0', | ||
advancedSettings: [], | ||
}, | ||
], | ||
[ | ||
'hydrateIrisGridPanelStateV1 one selected partition', | ||
{ | ||
isSelectingPartition: true, | ||
partition: '', | ||
partitionColumn: 'name_0', | ||
advancedSettings: [], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests don't seem right. A selected partition should have a value for partition,. and the "unselected partition" test - are we testing the empty string? Or isSelectingPartition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the first test is testing for isSelectingPartition
being false
(in which case the value of partition
) shouldn't matter and the second test is just testing for selecting a partition normally. partition
should be null
to signify a partition without a value so the second test is just testing for the empty string. I can use a non-empty string to make the test clearer.
@@ -680,3 +667,131 @@ describe('dehydration methods', () => { | |||
).toBe(true); | |||
}); | |||
}); | |||
|
|||
describe('hydration methods', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have cases for no partition column as well.
a9b210f
to
cfd3ff2
Compare
cfd3ff2
to
c80cd42
Compare
Added feature to
IrisGridPartitionSelector
to display additional partition selector dropdowns when more than one partition column is present. The values displayed in the dropdown depend on the selected partitions of columns to the left of it.The "Append Command" button and associated functions are removed in preparation for #1143.
Closes #1438
Testing instructions: