-
Notifications
You must be signed in to change notification settings - Fork 986
DRILL-8542: Support Paimon Format Plugin #3035
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Letian Jiang <[email protected]>
Signed-off-by: Letian Jiang <[email protected]>
cgivre
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.
@letian-jiang
Thank you very much for this submission. I did a quick first pass and overall it looks good.
General comments:
- You're overriding a lot of methods that I don't think actually need to be overridden. If the methods are not doing anything, I would suggest removing them.
- Again, thank you for providing robust unit tests. However, I made some refactoring suggestions for this class.
- Please avoid the x ? y : z notation for complex logic.
- For non-obvious functions, please add some documentation. We don't need docstrings for getters/setters and other obvious code, but most people working on Drill are likely unfamiliar with Paimon, so any help you could give in the code is greatly appreciated.
...src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatLocationTransformer.java
Outdated
Show resolved
Hide resolved
...ormat-paimon/src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatPlugin.java
Outdated
Show resolved
Hide resolved
...ormat-paimon/src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatPlugin.java
Show resolved
Hide resolved
...paimon/src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatPluginConfig.java
Show resolved
Hide resolved
| } | ||
| return null; | ||
| } | ||
| } |
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.
Nit: Please add blank line at the end of classes. Here and elsewhere.
contrib/format-paimon/src/test/java/org/apache/drill/exec/store/paimon/PaimonQueriesTest.java
Show resolved
Hide resolved
contrib/format-paimon/src/main/java/org/apache/drill/exec/store/paimon/PaimonWork.java
Show resolved
Hide resolved
Signed-off-by: Letian Jiang <[email protected]>
|
I believe I have addressed all the review comments. Could you please take another look? @cgivre |
...ormat-paimon/src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatPlugin.java
Show resolved
Hide resolved
...ormat-paimon/src/main/java/org/apache/drill/exec/store/paimon/format/PaimonFormatPlugin.java
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| } |
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.
Please add new line at the end of all classes. Here and elsewhere..
| import org.apache.drill.common.exceptions.UserException; | ||
| import org.apache.drill.common.expression.LogicalExpression; | ||
| import org.apache.drill.common.expression.SchemaPath; | ||
| import org.apache.drill.exec.physical.impl.scan.framework.ManagedReader; |
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.
Note: This is using the older version of the ManagedReader class. Please use org.apache.drill.exec.physical.impl.scan.v3.ManagedReader. This will require refactoring this class a bit.
| } | ||
|
|
||
| @Test | ||
| public void testSelectManifestsMetadata() throws Exception { |
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.
Looking good. Can you please add a SerDe test? If this test fails, you won't be able to update the config. Take a look here as an example. :
drill/contrib/format-excel/src/test/java/org/apache/drill/exec/store/excel/TestExcelFormat.java
Lines 564 to 570 in bcb4386
| public void testSerDe() throws Exception { | |
| String sql = "SELECT COUNT(*) as cnt FROM table(cp.`excel/test_data.xlsx` (type=> 'excel', sheetName => 'inconsistentData', allTextMode => true))"; | |
| String plan = queryBuilder().sql(sql).explainJson(); | |
| long cnt = queryBuilder().physical(plan).singletonLong(); | |
| assertEquals("Counts should match",4L, cnt); | |
| } | |
DRILL-8542: Support Paimon format plugin
Description
Introduce a Paimon format plugin, enabling Drill users to query Paimon tables directly via filesystem paths.
Usage examples:
/path/to/paimon_table/path/to/paimon_table, snapshotId => 123)/path/to/paimon_table, snapshotAsOfTime => 1700000000000)/path/to/paimon_table#snapshotsReference: