Skip to content
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

Add data file root location in DataFileMeta #4751

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

neuyilan
Copy link
Member

@neuyilan neuyilan commented Dec 21, 2024

@wwj6591812
Copy link
Contributor

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 =
Copy link
Contributor

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.

Copy link
Member Author

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;
Copy link
Contributor

@JingsongLi JingsongLi Dec 22, 2024

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

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to externalPath?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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.

@neuyilan neuyilan closed this Dec 22, 2024
@neuyilan neuyilan reopened this Dec 22, 2024
@neuyilan
Copy link
Member Author

Please add the Purpose of this PR.
done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants