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/09/13 09:09:11 UTC
[GitHub] [flink-table-store] SteNicholas opened a new pull request, #292: [FLINK-29278] BINARY type is not supported in table store
SteNicholas opened a new pull request, #292:
URL: https://github.com/apache/flink-table-store/pull/292
`BINARY` type is not supported in table store for `OrcFileFormat`.
**The brief change log**
- Adds the `BINARY` type support for `OrcFileFormat`.
--
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 #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#discussion_r969385589
##########
flink-table-store-common/src/test/java/org/apache/flink/table/store/format/FileStatsExtractorTestBase.java:
##########
@@ -128,6 +129,9 @@ private Object createField(LogicalType type) {
randomString(random.nextInt(varCharType.getLength()) + 1));
case BOOLEAN:
return random.nextBoolean();
+ case BINARY:
Review Comment:
I think we should add a case in `FileStoreTableTestBase` to test read and write.
--
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] SteNicholas commented on pull request #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#issuecomment-1253416018
@JingsongLi, I have addressed above comments and added the `BinaryType` and `VarBinaryType` types for `FileStoreTableTestBase`. 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] JingsongLi commented on a diff in pull request #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#discussion_r969376212
##########
flink-table-store-format/src/main/java/org/apache/flink/table/store/format/orc/OrcFileFormat.java:
##########
@@ -108,4 +113,71 @@ private static Properties getOrcProperties(ReadableConfig options) {
properties.forEach((k, v) -> orcProperties.put(IDENTIFIER + "." + k, v));
return orcProperties;
}
+
+ /** See {@code org.apache.flink.table.catalog.hive.util.HiveTypeUtil}. */
+ public static TypeDescription logicalTypeToOrcType(LogicalType type) {
+ type = type.copy(true);
+ switch (type.getTypeRoot()) {
+ case CHAR:
+ return TypeDescription.createChar().withMaxLength(((CharType) type).getLength());
+ case VARCHAR:
+ int len = ((VarCharType) type).getLength();
+ if (len == VarCharType.MAX_LENGTH) {
+ return TypeDescription.createString();
+ } else {
+ return TypeDescription.createVarchar().withMaxLength(len);
+ }
+ case BOOLEAN:
+ return TypeDescription.createBoolean();
+ case BINARY:
+ return TypeDescription.createBinary();
+ case VARBINARY:
+ if (type.equals(DataTypes.BYTES().getLogicalType())) {
+ return TypeDescription.createBinary();
+ } else {
+ throw new UnsupportedOperationException(
Review Comment:
We can support this 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 merged pull request #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
JingsongLi merged PR #292:
URL: https://github.com/apache/flink-table-store/pull/292
--
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] SteNicholas commented on a diff in pull request #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
SteNicholas commented on code in PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#discussion_r970647115
##########
flink-table-store-common/src/test/java/org/apache/flink/table/store/format/FileStatsExtractorTestBase.java:
##########
@@ -128,6 +129,9 @@ private Object createField(LogicalType type) {
randomString(random.nextInt(varCharType.getLength()) + 1));
case BOOLEAN:
return random.nextBoolean();
+ case BINARY:
Review Comment:
@JingsongLi, the `testReadFilter` in `FileStoreTableTestBase` could cover the test case to test read and write. 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] JingsongLi commented on a diff in pull request #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#discussion_r974118665
##########
flink-table-store-common/src/test/java/org/apache/flink/table/store/format/FileStatsExtractorTestBase.java:
##########
@@ -128,6 +129,9 @@ private Object createField(LogicalType type) {
randomString(random.nextInt(varCharType.getLength()) + 1));
case BOOLEAN:
return random.nextBoolean();
+ case BINARY:
Review Comment:
We can add test for new types.
--
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 #292: [FLINK-29278] BINARY type is not supported in table store
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #292:
URL: https://github.com/apache/flink-table-store/pull/292#discussion_r976264399
##########
flink-table-store-format/src/main/java/org/apache/flink/table/store/format/orc/OrcFileFormat.java:
##########
@@ -115,4 +124,96 @@ private static Properties getOrcProperties(ReadableConfig options) {
properties.forEach((k, v) -> orcProperties.put(IDENTIFIER + "." + k, v));
return orcProperties;
}
+
+ private static LogicalType convertLogicalType(LogicalType type) {
Review Comment:
can we change a method name? or add some comments to explain `BINARY` and `VARBINARY`
##########
flink-table-store-format/src/main/java/org/apache/flink/table/store/format/orc/OrcFileFormat.java:
##########
@@ -92,14 +98,17 @@ public BulkFormat<RowData, FileSourceSplit> createReaderFactory(
}
return OrcInputFormatFactory.create(
- readerConf, type, Projection.of(projection).toTopLevelIndexes(), orcPredicates);
+ readerConf,
+ (RowType) convertLogicalType(type),
+ Projection.of(projection).toTopLevelIndexes(),
+ orcPredicates);
}
@Override
public BulkWriter.Factory<RowData> createWriterFactory(RowType type) {
LogicalType[] orcTypes = type.getChildren().toArray(new LogicalType[0]);
- TypeDescription typeDescription = OrcSplitReaderUtil.logicalTypeToOrcType(type);
+ TypeDescription typeDescription = logicalTypeToOrcType(type);
Review Comment:
can we use `OrcSplitReaderUtil.logicalTypeToOrcType(convertLogicalType(type))`?
--
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