DRILL-8542: Support Paimon Format Plugin#3035
Conversation
Signed-off-by: Letian Jiang <letian.jiang@outlook.com>
Signed-off-by: Letian Jiang <letian.jiang@outlook.com>
cgivre
left a comment
There was a problem hiding this comment.
@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.
Nit: Please add blank line at the end of classes. Here and elsewhere.
There was a problem hiding this comment.
I was referring to the end of each file. After the last closing brace, there should be a newline.
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 <letian.jiang@outlook.com>
|
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.
Please add new line at the end of all classes. Here and elsewhere..
There was a problem hiding this comment.
Do you mean adding a new line for (inner) classes? This is precisely what is desired.
.../format-paimon/src/main/java/org/apache/drill/exec/store/paimon/read/PaimonRecordReader.java
Outdated
Show resolved
Hide resolved
contrib/format-paimon/src/test/java/org/apache/drill/exec/store/paimon/PaimonQueriesTest.java
Show resolved
Hide resolved
Signed-off-by: Letian Jiang <letian.jiang@outlook.com>
|
Please check this again. @cgivre |
cgivre
left a comment
There was a problem hiding this comment.
@letian-jiang This looks good. Thanks for the contribution!
Could you please verify that there is a new line at the end of each file? Once that is done and CI passes, I'll merge this!
Lastly, could you also add documentation to the Drill documentation site? (https://github.com/apache/drill-site).
I don't know if you have any connection the Paimon team, but if so, a link to Drill would also be greatly appreciated.
LGTM +1
I have already verified it. Let me see how to add the document. |
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: