You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "puchengy (via GitHub)" <gi...@apache.org> on 2023/10/17 20:23:42 UTC
[PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
puchengy opened a new pull request, #83:
URL: https://github.com/apache/iceberg-python/pull/83
We sanitized column name to meet the Avro restriction when writing parquet data (https://github.com/apache/iceberg/blob/ad602a379584512d1d96eda557c20cf2af21d1b2/core/src/main/java/org/apache/iceberg/avro/AvroSchemaUtil.java#L429).
This PR fix issues https://github.com/apache/iceberg-python/issues/81.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
Posted by "puchengy (via GitHub)" <gi...@apache.org>.
puchengy commented on PR #83:
URL: https://github.com/apache/iceberg-python/pull/83#issuecomment-1767128295
@Fokko Can I get a review? Thanks
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
Posted by "puchengy (via GitHub)" <gi...@apache.org>.
puchengy commented on code in PR #83:
URL: https://github.com/apache/iceberg-python/pull/83#discussion_r1365869931
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ return False
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ return False
Review Comment:
thanks, I actually asked chatgpt to rewrite the java implementation in iceberg repo to me, so it is not knowing iceberg-python styling.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
Posted by "puchengy (via GitHub)" <gi...@apache.org>.
puchengy commented on code in PR #83:
URL: https://github.com/apache/iceberg-python/pull/83#discussion_r1365868994
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
Review Comment:
I think we do not allow empty names, more importantly, I just copy this from Iceberg implementation so we don't bother to figure this out.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #83:
URL: https://github.com/apache/iceberg-python/pull/83#discussion_r1363628652
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
Review Comment:
We avoid assertions in the code itself. If we want to check this, please raise a `ValueError` (or omit the check, do we allow empty names at all?)
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ return False
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ return False
Review Comment:
We could achieve the same with a slice:
```suggestion
for character in name[1:]:
if not (character.isalnum() or character == '_'):
return False
```
Example:
```
python3
Python 3.11.6 (main, Oct 2 2023, 13:45:54) [Clang 15.0.0 (clang-1500.0.40.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> "abc"[1:]
'bc'
```
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ return False
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ return False
+ return True
+
+
+def _sanitize_name(name: str) -> str:
+ length = len(name)
+ sb = []
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ sb.append(_sanitize_char(first))
+ else:
+ sb.append(first)
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ sb.append(_sanitize_char(character))
+ else:
+ sb.append(character)
+ return ''.join(sb)
+
+
+def _sanitize_char(character: str) -> str:
+ if character.isdigit():
+ return "_" + character
+ return "_x" + hex(ord(character))[2:].upper()
Review Comment:
```suggestion
return "_" + character if character.isdigit() else "_x" + hex(ord(character))[2:].upper()
```
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ return False
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ return False
+ return True
+
+
+def _sanitize_name(name: str) -> str:
+ length = len(name)
+ sb = []
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ sb.append(_sanitize_char(first))
+ else:
+ sb.append(first)
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ sb.append(_sanitize_char(character))
+ else:
+ sb.append(character)
Review Comment:
We can also use the slice here:
```suggestion
for character in name[1:]:
if not (character.isalnum() or character == '_'):
sb.append(_sanitize_char(character))
else:
sb.append(character)
```
Or:
```suggestion
for character in name[1:]:
sb.append(_sanitize_char(character) if not (character.isalnum() or character == '_') else character)
```
Or:
```suggestion
sb = [first] + [
_sanitize_char(character) if not (character.isalnum() or character == '_') else character
]
```
##########
pyiceberg/schema.py:
##########
@@ -1273,6 +1273,102 @@ def primitive(self, primitive: PrimitiveType) -> PrimitiveType:
return primitive
+# Implementation copied from Apache Iceberg repo.
+def make_compatible_name(name: str) -> str:
+ if not _valid_avro_name(name):
+ return _sanitize_name(name)
+ return name
+
+
+def _valid_avro_name(name: str) -> bool:
+ length = len(name)
+ assert length > 0, "Empty name"
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ return False
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ return False
+ return True
+
+
+def _sanitize_name(name: str) -> str:
+ length = len(name)
+ sb = []
+ first = name[0]
+ if not (first.isalpha() or first == '_'):
+ sb.append(_sanitize_char(first))
+ else:
+ sb.append(first)
+
+ for i in range(1, length):
+ character = name[i]
+ if not (character.isalnum() or character == '_'):
+ sb.append(_sanitize_char(character))
+ else:
+ sb.append(character)
+ return ''.join(sb)
+
+
+def _sanitize_char(character: str) -> str:
+ if character.isdigit():
+ return "_" + character
+ return "_x" + hex(ord(character))[2:].upper()
+
+
+def sanitize_column_names(schema: Schema) -> Schema:
+ """Sanitize column names to make them compatible with Avro.
Review Comment:
Maybe we can add here what Avro doesn't accept.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make to_arrow function capable of handling parquet files with sanitized name due to Avro restirction [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #83:
URL: https://github.com/apache/iceberg-python/pull/83
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org