-
Notifications
You must be signed in to change notification settings - Fork 454
[lake/paimon] Support Array types for tiering paimon #2166
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
base: main
Are you sure you want to change the base?
[lake/paimon] Support Array types for tiering paimon #2166
Conversation
|
added few mores tests cases hoping coverage will be okay |
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.
Pull request overview
This PR implements support for ARRAY and nested ARRAY data types in the Fluss Lake Paimon tiering service, addressing issue #1980. Previously, attempting to use array types would result in an UnsupportedOperationException. The implementation provides bidirectional conversion between Fluss and Paimon array representations through adapter classes.
- Introduced two adapter classes to enable seamless array type conversion between Fluss and Paimon
- Updated row conversion methods to return wrapped array instances instead of throwing exceptions
- Added comprehensive test coverage with 35 new test methods across three test files
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| FlussArrayAsPaimonArray.java | New adapter class that wraps Fluss InternalArray to implement Paimon InternalArray interface, enabling Fluss→Paimon array conversion with support for all primitive types, decimals, timestamps, and nested arrays |
| PaimonArrayAsFlussArray.java | New adapter class that wraps Paimon InternalArray to implement Fluss InternalArray interface, enabling Paimon→Fluss array conversion with proper handling of both TimestampNtz and TimestampLtz types |
| FlussRowAsPaimonRow.java | Updated getArray() method to return FlussArrayAsPaimonArray wrapper instead of throwing UnsupportedOperationException |
| PaimonRowAsFlussRow.java | Updated getArray() method to return PaimonArrayAsFlussArray wrapper instead of throwing UnsupportedOperationException |
| PaimonRowAsFlussRowTest.java | New test class with 15 comprehensive test methods validating Paimon→Fluss array conversion including primitives, strings, decimals, timestamps, null handling, and nested arrays |
| FlussRecordAsPaimonRowTest.java | Added 10 test methods verifying array support in the tiering context with system columns, covering all primitive types and edge cases |
| FlussRowAsPaimonRowTest.java | Added 10 test methods validating Fluss→Paimon array conversion for various data types including nested arrays, null values, and empty arrays |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| InternalArray innerArray2 = outerArray.getArray(1); | ||
| assertThat(innerArray2.size()).isEqualTo(3); | ||
| assertThat(innerArray2.getInt(0)).isEqualTo(3); | ||
| } |
Copilot
AI
Dec 15, 2025
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.
The test assertions for innerArray2 are incomplete. The array contains 3 elements (3, 4, 5) based on the test setup at line 266, but only the first element is verified. The assertions for elements at indices 1 and 2 are missing, making this test less comprehensive than the equivalent tests in PaimonRowAsFlussRowTest and FlussRowAsPaimonRowTest where all elements are verified.
luoyuxia
left a comment
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.
@binary-signal Thanks for the pr.
Also add IT to vierfy array type can be tiered to paimon and unioin read by flink.
Maybe you can add the it in FlinkUnionReadPrimaryKeyTableITCase#testUnionReadFullType
| } | ||
|
|
||
| @Test | ||
| void testArrayWithAllPrimitiveTypes() { |
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.
is it possible to put test for all types in array in this one test method instead having testArrayWithLongElements, testArrayWithDoubleElements, etc...
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.
Yes that make sense I am on it
| } | ||
|
|
||
| @Test | ||
| void testArrayWithAllPrimitiveTypes() { |
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.
dito. Is it possible to test in one single test method instead of introduing many other test method
| } | ||
|
|
||
| @Test | ||
| void testArrayWithAllPrimitiveTypes() { |
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.
dito.
Is it possible to reduce test methods?
|
Hi @binary-signal , do you have time to update the pull request? If you're busy, we’d be happy to help with the changes and get it merged. |
|
@luoyuxia could you help to update the PR and I will make a final review. |
|
@wuchong Sure. I'll help update the pr |
Signed-off-by: binary-signal <binary-signal@github.noreply.com>
Signed-off-by: binary-signal <binary-signal@github.noreply.com>
Signed-off-by: binary-signal <binary-signal@github.noreply.com>
bb18842 to
c22e6b7
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.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...s-lake-paimon/src/main/java/org/apache/fluss/lake/paimon/source/FlussArrayAsPaimonArray.java
Outdated
Show resolved
Hide resolved
...ss-lake-paimon/src/test/java/org/apache/fluss/lake/paimon/utils/PaimonRowAsFlussRowTest.java
Outdated
Show resolved
Hide resolved
...s-lake-paimon/src/test/java/org/apache/fluss/lake/paimon/source/FlussRowAsPaimonRowTest.java
Outdated
Show resolved
Hide resolved
c22e6b7 to
245e87d
Compare
|
@wuchong Hi, i already updated the pr. Please help review when you got some time |
Purpose
Linked Issue: #2250
This PR adds support for ARRAY data types in the Fluss Lake Paimon integration. Previously, attempting to use array types when tiering data to Paimon would throw an
UnsupportedOperationException. This change enables proper conversion between Fluss and Paimon array types in both directions.Brief change log
Added
FlussArrayAsPaimonArray: Adapter class that wrapsorg.apache.fluss.row.InternalArrayand implementsorg.apache.paimon.data.InternalArray, enabling Fluss arrays to be read as Paimon arrays.Added
PaimonArrayAsFlussArray: Adapter class that wrapsorg.apache.paimon.data.InternalArrayand implementsorg.apache.fluss.row.InternalArray, enabling Paimon arrays to be read as Fluss arrays.Updated
FlussRowAsPaimonRow.getArray(): Now returns aFlussArrayAsPaimonArraywrapper instead of throwingUnsupportedOperationException.Updated
PaimonRowAsFlussRow.getArray(): Now returns aPaimonArrayAsFlussArraywrapper instead of throwingUnsupportedOperationException.Tests
FlussRowAsPaimonRowTest (10 new test methods):
testArrayTypeWithIntElements()- Verifies array of integers can be read correctlytestArrayTypeWithStringElements()- Verifies array of strings can be read correctlytestArrayTypeWithNullableElements()- Verifies arrays with null elements are handled properlytestNullArray()- Verifies null array fields returnisNullAt() == truetestNestedArrayType()- Verifies nested arrays (ARRAY<ARRAY<INT>>) work correctlytestEmptyArray()- Verifies empty arrays are handled correctlytestArrayWithAllPrimitiveTypes()- Verifies all primitive array types andtoXArray()conversionstestArrayWithDecimalElements()- VerifiesARRAY<DECIMAL>type conversiontestArrayWithTimestampElements()- VerifiesARRAY<TIMESTAMP>type conversiontestArrayWithBinaryElements()- VerifiesARRAY<VARBINARY>type conversionPaimonRowAsFlussRowTest (new test class - 15 test methods):
testArrayTypeWithIntElements()- Verifies Paimon int arrays convert to Fluss correctlytestArrayTypeWithStringElements()- Verifies Paimon string arrays convert to Fluss correctlytestArrayTypeWithNullableElements()- Verifies arrays with null elements are handledtestNullArray()- Verifies null array fields are handledtestNestedArrayType()- Verifies nested arrays work in Paimon→Fluss directiontestEmptyArray()- Verifies empty arrays are handledtestReplaceRow()- VerifiesreplaceRow()works correctly with array fieldstestArrayWithLongElements()- VerifiesARRAY<BIGINT>conversiontestArrayWithDoubleElements()- VerifiesARRAY<DOUBLE>conversiontestArrayWithBooleanElements()- VerifiesARRAY<BOOLEAN>conversiontestArrayWithAllPrimitiveTypes()- Verifies all primitive types andtoXArray()methodstestArrayWithDecimalElements()- VerifiesARRAY<DECIMAL>conversiontestArrayWithTimestampElements()- Verifies bothgetTimestampNtz()andgetTimestampLtz()testArrayWithBinaryElements()- Verifies bothgetBinary()andgetBytes()methodstestArrayWithCharElements()- Verifies bothgetChar()andgetString()methodsFlussRecordAsPaimonRowTest (10 new test methods):
testArrayTypeWithIntElements()- Verifies int arrays work with system columns in tiering contexttestArrayTypeWithStringElements()- Verifies string arrays work in tiering contexttestNestedArrayType()- Verifies nested arrays work in tiering contexttestArrayWithAllPrimitiveTypes()- Verifies all primitive types andtoXArray()methods with system columnstestArrayWithDecimalElements()- VerifiesARRAY<DECIMAL>in tiering contexttestArrayWithTimestampElements()- VerifiesARRAY<TIMESTAMP>in tiering contexttestArrayWithBinaryElements()- VerifiesARRAY<VARBINARY>in tiering contexttestNullArray()- Verifies null arrays in tiering contexttestArrayWithNullableElements()- Verifies arrays with null elements in tiering contexttestEmptyArray()- Verifies empty arrays in tiering contextAPI and Format
No. This change is an internal implementation that enables existing array type support to work correctly with Paimon tiering. No public API changes.
Documentation
No documentation changes required. Array types were already documented as a supported data type; this PR fixes the implementation to actually support them in the Paimon lake integration.