You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@parquet.apache.org by GitBox <gi...@apache.org> on 2021/04/01 12:33:40 UTC
[GitHub] [parquet-format] pitrou opened a new pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
pitrou opened a new pull request #169:
URL: https://github.com/apache/parquet-format/pull/169
Also slight wording improvements, and replace a "must" with "should" for writing the ConvertedType field.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] pitrou merged pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
pitrou merged pull request #169:
URL: https://github.com/apache/parquet-format/pull/169
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] emkornfield commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606825680
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
sorry for the belated comment by why the change from must to should?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] pitrou commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606831456
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
That said, feel free to spawn a discussion on the ML.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] wesm commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606836868
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
It's a change with forward compatibility implications, so either we revert or we discuss/vote, but we can't leave it like this. Impala for example only started to get support for LogicalType in 2019 (and this is only one of the mainstream independent implementations), so we'd be making a bet about how quickly updates are rolling out to all of the corners of the world
https://github.com/apache/impala/commit/0906e0817ce2301d7e20f355b334861f0232f16f
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] pitrou commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606835027
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
That said, if people feel strongly about it, I can simply revert this one-word change, because I don't think there's any point organizing a vote on it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] pitrou commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606833694
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
Well, given the state, even today, of inter-implementation compatibility, I think anyone using a 2017-era implementation would run into lots of issues when reading files produced by other implementations, regardless of whether logical types are used.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] pitrou commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606831346
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
Logical types were introduced 3.5 years go, so it seemed reasonable to relax the requirement. Do you expect some Parquet readers in the wild to still not understand them?
It's easy, for every converted type you support, to also support the corresponding logical type. It's only a small amount of logic in the decoding path, and it's not performance-critical.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] wesm commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606826402
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
I also believed that setting ConvertedType was MUST for forward compatibility reasons. Can we please discuss on the mailing list?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [parquet-format] wesm commented on a change in pull request #169: PARQUET-2013: [Format] Mention that ConvertedType is deprecated
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #169:
URL: https://github.com/apache/parquet-format/pull/169#discussion_r606832265
##########
File path: src/main/thrift/parquet.thrift
##########
@@ -316,7 +317,7 @@ struct BsonType {
* LogicalType annotations to replace ConvertedType.
*
* To maintain compatibility, implementations using LogicalType for a
- * SchemaElement must also set the corresponding ConvertedType from the
+ * SchemaElement should also set the corresponding ConvertedType from the
Review comment:
If you want to make this change, I think it needs to be voted on. Knowing the motley state of Parquet implementations in enterprise deployments I would be uncomfortable writing files that don't have the field.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org