You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/20 13:17:43 UTC
[GitHub] [flink-table-store] hililiwei opened a new pull request, #133: [FLINK-27502] Add file Suffix to data files
hililiwei opened a new pull request, #133:
URL: https://github.com/apache/flink-table-store/pull/133
## What is the purpose of the change
Add file Suffix to data files
## Verifying this change
org.apache.flink.table.store.format.FileFormatSuffixTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#discussion_r878786902
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/format/FileFormat.java:
##########
@@ -53,10 +53,20 @@
*/
public abstract class FileFormat {
+ protected String formatIdentifier;
+
+ protected FileFormat(String formatIdentifier) {
+ this.formatIdentifier = formatIdentifier;
+ }
+
protected abstract BulkDecodingFormat<RowData> getDecodingFormat();
protected abstract EncodingFormat<BulkWriter.Factory<RowData>> getEncodingFormat();
+ public String getSuffix() {
Review Comment:
Can we unify the naming? I see a lot of places where it's `formatIdentifier`, but here it's `getSuffix`.
We can use `getFormatIdentifier` here.
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/data/DataFilePathFactory.java:
##########
@@ -34,20 +34,30 @@ public class DataFilePathFactory {
private final String uuid;
private final AtomicInteger pathCount;
+ private final String formatIdentifier;
public DataFilePathFactory(Path root, String partition, int bucket) {
Review Comment:
I think we can do without this constructor, the `formatIdentifier` is not a nullable field, and the following can be done without having the special treatment of null.
##########
flink-table-store-format/pom.xml:
##########
@@ -190,6 +190,13 @@ under the License.
</exclusion>
</exclusions>
</dependency>
+
+ <dependency>
+ <groupId>junit</groupId>
Review Comment:
Do we need this dependency? I see you are using junit 5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1141977492
Updated and passed the fork repo CI. Please let it run again, i will see if there are any other mistakes. @JingsongLi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on a diff in pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#discussion_r883271494
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/utils/FileStorePathFactory.java:
##########
@@ -106,7 +114,12 @@ public Path toTmpSnapshotPath(long id) {
}
public DataFilePathFactory createDataFilePathFactory(BinaryRowData partition, int bucket) {
- return new DataFilePathFactory(root, getPartitionString(partition), bucket);
+ return createDataFilePathFactory(partition, bucket, formatIdentifier);
+ }
+
+ public DataFilePathFactory createDataFilePathFactory(
Review Comment:
Sorry, I submitted the wrong one. Has been updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1138531071
> Thanks @hililiwei for the contribution! Looks nice~ I left some comments.
thank you for review, PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on a diff in pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#discussion_r882623699
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/data/DataFilePathFactory.java:
##########
@@ -34,20 +34,30 @@ public class DataFilePathFactory {
private final String uuid;
private final AtomicInteger pathCount;
+ private final String formatIdentifier;
public DataFilePathFactory(Path root, String partition, int bucket) {
Review Comment:
Fair enough, deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1139217330
You can run a `mvn spotless:apply` after modification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on a diff in pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#discussion_r882622802
##########
flink-table-store-format/pom.xml:
##########
@@ -190,6 +190,13 @@ under the License.
</exclusion>
</exclusions>
</dependency>
+
+ <dependency>
+ <groupId>junit</groupId>
Review Comment:
deleted
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/format/FileFormat.java:
##########
@@ -53,10 +53,20 @@
*/
public abstract class FileFormat {
+ protected String formatIdentifier;
+
+ protected FileFormat(String formatIdentifier) {
+ this.formatIdentifier = formatIdentifier;
+ }
+
protected abstract BulkDecodingFormat<RowData> getDecodingFormat();
protected abstract EncodingFormat<BulkWriter.Factory<RowData>> getEncodingFormat();
+ public String getSuffix() {
Review Comment:
good, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1139279223
> You can run a `mvn spotless:apply` after modification.
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1140603513
```
Error: Failures:
Error: DataFilePathFactoryTest.testNoPartition:46
expected: /tmp/junit13044672734532131/bucket-123/data-de33b78b-2802-49a0-8ec7-cbeeed7af049-0
but was: /tmp/junit13044672734532131/bucket-123/data-de33b78b-2802-49a0-8ec7-cbeeed7af049-0.orc
Error: DataFilePathFactoryTest.testWithPartition:64
expected: /tmp/junit3939205851475787579/dt=20211224/bucket-123/data-8a204f50-ad09-4e55-a41f-39c94da0863c-0
but was: /tmp/junit3939205851475787579/dt=20211224/bucket-123/data-8a204f50-ad09-4e55-a41f-39c94da0863c-0.orc
[INFO]
```
Test failure, you need to adjust some testing codes too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#discussion_r883213162
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/utils/FileStorePathFactory.java:
##########
@@ -106,7 +114,12 @@ public Path toTmpSnapshotPath(long id) {
}
public DataFilePathFactory createDataFilePathFactory(BinaryRowData partition, int bucket) {
- return new DataFilePathFactory(root, getPartitionString(partition), bucket);
+ return createDataFilePathFactory(partition, bucket, formatIdentifier);
+ }
+
+ public DataFilePathFactory createDataFilePathFactory(
Review Comment:
Why introduce this method? There is a `formatIdentifier` in here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] hililiwei commented on pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #133:
URL: https://github.com/apache/flink-table-store/pull/133#issuecomment-1149544053
thanks for review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi merged pull request #133: [FLINK-27502] Add file Suffix to data files
Posted by GitBox <gi...@apache.org>.
JingsongLi merged PR #133:
URL: https://github.com/apache/flink-table-store/pull/133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org