-
Notifications
You must be signed in to change notification settings - Fork 988
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
[Core]Add data file root location in DataFileMeta #4751
base: master
Are you sure you want to change the base?
Conversation
Please add the Purpose of this PR. |
@@ -131,6 +131,20 @@ public class CoreOptions implements Serializable { | |||
.noDefaultValue() | |||
.withDescription("The file path of this table in the filesystem."); | |||
|
|||
@ExcludeFromDocumentation("Internal use only") | |||
public static final ConfigOption<String> WAREHOUSE_ROOT_PATH = |
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.
Can you not introduce any option in this pr? Just modify data file meta.
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.
ok
* warehouse path, when {@link CoreOptions#DATA_FILE_PATH_DIRECTORY} is set, new writen files | ||
* will be persisted in {@link CoreOptions#DATA_FILE_PATH_DIRECTORY}. | ||
*/ | ||
private final @Nullable String dataRootLocation; |
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.
We need to introduce a new version for CommitMessage and DataSplit.
You can refer to #4322
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 see that since version 0.9 [1], the versions of commitMessage and DataSplit have been changed. Do I need to make another change? I think it only needs to be changed once in version 1.0.
[1] https://github.com/apache/paimon/blob/release-0.9/paimon-core/src/main/java/org/apache/paimon/table/source/DataSplit.java
[1] https://github.com/apache/paimon/blob/release-0.9/paimon-core/src/main/java/org/apache/paimon/table/sink/CommitMessageSerializer.java
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.
Keep compatibility, even it is just in 1.0-SNAPSHOT, for example, creating DataFileMeta10LegacySerializer
for previous DataFileMetaSerializer.
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.
Understood, if it's for compatibility with 1.0-SNAPSHOT, it's worth doing this.
* warehouse path, when {@link CoreOptions#DATA_FILE_PATH_DIRECTORY} is set, new writen files | ||
* will be persisted in {@link CoreOptions#DATA_FILE_PATH_DIRECTORY}. | ||
*/ | ||
private final @Nullable String dataRootLocation; |
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.
rename to externalPath?
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.
No problem, I'm willing to express that this path is the same root path as the warehouse, not the full path. But it doesn't affect the modification of DataFileMeta this time. I'll change it to extrenalPath first
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.
maybe it is better to just full path?
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 will think deeply about it in the upcoming PR, but it does not currently affect the functionality of this PR.
|
Purpose
This is part of the job[1]
[1] https://docs.google.com/document/d/1NhmOyxM16QmY_rVb3KJtCKRrU_nogIJv532U59qW7EI/edit?tab=t.0#heading=h.xlrl29nlxwpo
Tests
API and Format
Documentation