You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/06/29 22:38:24 UTC
[GitHub] [iceberg] holdenk opened a new pull request #2761: Add an implemention of STARTS_WITH for Python API.
holdenk opened a new pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761
This is a follow up of (follow up for https://github.com/apache/iceberg/pull/327 filed in https://github.com/apache/iceberg/issues/2019 ).
This is my first time working on the Python code in Iceberg. Implementation wise I based this on NOT_EQ.
For Python API compat this exposes both `starts_with` (which is the snake case version of the Java API startsWith) and `startswith` the name of the logical similar method in the Python string library.
--
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
[GitHub] [iceberg] holdenk removed a comment on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk removed a comment on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-885933174
nvm my bad.
--
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
[GitHub] [iceberg] TGooch44 commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-889045179
> I think there might be some arrow stuff to add, but that might be outdated. A few comments.
I think i may have mistakenly mentioned the arrow_table_utils, which doesn't exist in open source, sorry for the confusion.
--
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
[GitHub] [iceberg] TGooch44 commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-871719283
@holdenk I'll review this ASAP
--
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
[GitHub] [iceberg] holdenk commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r661889045
##########
File path: python/iceberg/api/expressions/expressions.py
##########
@@ -106,6 +106,15 @@ def equal(name, value):
def not_equal(name, value):
return UnboundPredicate(Operation.NOT_EQ, Expressions.ref(name), value)
+ # In Python we use "startswith", to match the direct conversion & the pythonic impl do both.
Review comment:
Yeah so good point on that being confusing.
startswith is to match the Python impl and starts_with is to match the "normal" conversion of camel case to snake case that is present in PyIceberg. I'll update the comment, 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
[GitHub] [iceberg] kbendick commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-890608366
> > I think there might be some arrow stuff to add, but that might be outdated. A few comments.
>
> I think i may have mistakenly mentioned the arrow_table_utils, which doesn't exist in open source, sorry for the confusion.
Ah that explains it. I went looking for it, hence the "might be outdated". Thanks for clarifying :)
--
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
[GitHub] [iceberg] holdenk commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r679363896
##########
File path: python/iceberg/api/expressions/inclusive_manifest_evaluator.py
##########
@@ -159,3 +159,7 @@ def in_(self, ref, lit):
def not_in(self, ref, lit):
return ROWS_MIGHT_MATCH
+
+ def starts_with(self, ref, lit):
+ # if it's all null then we can't match :)
+ return self.not_null(ref)
Review comment:
I don't have strong views here either, my only consideration with changing it to `return ROWS_CANNOT_MATCH if self.not_null(ref) else ROWS_MIGHT_MATCH` would be if we ever change this from wrapping ROWS_CANNOT_MATCH/ROWS_MIGHT_MATCH true/false it would brake but we'd catch 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.
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
[GitHub] [iceberg] kbendick commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r678890318
##########
File path: python/iceberg/api/expressions/expression_parser.py
##########
@@ -44,7 +44,7 @@
ident = Word(alphas, alphanums + "_$").setName("identifier")
columnName = delimitedList(ident, ".", combine=True).setName("column name")
-binop = oneOf("= == != < > >= <= eq ne lt le gt ge <>", caseless=False)
+binop = oneOf("= == != < > >= <= eq ne lt le gt ge <> startsWith", caseless=False)
Review comment:
Would it make sense to allow for `starts_with` in the expression parser too? The SQL wouldn't exactly be portable outside of PySpark etc, but users might want to use the language natural `starts_with` and we already have `lt` `le` and their counterparts `<` `<=` etc.
##########
File path: python/iceberg/api/expressions/inclusive_manifest_evaluator.py
##########
@@ -159,3 +159,7 @@ def in_(self, ref, lit):
def not_in(self, ref, lit):
return ROWS_MIGHT_MATCH
+
+ def starts_with(self, ref, lit):
+ # if it's all null then we can't match :)
+ return self.not_null(ref)
Review comment:
Most of the functions seem to explicitly return the constants `ROWS_MIGHT_MATCH` and `ROWS_CANNOT_MATCH`. Would this fit in more as `return ROWS_CANNOT_MATCH if self.not_null(ref) else ROWS_MIGHT_MATCH` or similar?
--
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
[GitHub] [iceberg] holdenk edited a comment on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk edited a comment on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-885933174
nvm my bad.
--
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
[GitHub] [iceberg] TGooch44 commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r679070069
##########
File path: python/iceberg/api/expressions/expression_parser.py
##########
@@ -44,7 +44,7 @@
ident = Word(alphas, alphanums + "_$").setName("identifier")
columnName = delimitedList(ident, ".", combine=True).setName("column name")
-binop = oneOf("= == != < > >= <= eq ne lt le gt ge <>", caseless=False)
+binop = oneOf("= == != < > >= <= eq ne lt le gt ge <> startsWith", caseless=False)
Review comment:
> I think there might be some arrow stuff to add, but that might be outdated. A few comments.
Was there something you are thinking of specifically? I think just the dataset filter translation is all that's needed for what's currently in the read path. I could be missing something though.
--
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
[GitHub] [iceberg] kbendick commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r661768686
##########
File path: python/iceberg/api/expressions/expressions.py
##########
@@ -106,6 +106,15 @@ def equal(name, value):
def not_equal(name, value):
return UnboundPredicate(Operation.NOT_EQ, Expressions.ref(name), value)
+ # In Python we use "startswith", to match the direct conversion & the pythonic impl do both.
Review comment:
Perhaps I haven't had enough coffee yet, but the ending of this phrase is hard for me to grok where it says "to match the direct conversion" is a bit hard for me to grok.
Please correct me if I'm wrong, but I think it means that we use `starts_with` to match the normal Java Iceberg API of `startsWith`, but allow for `startswith` to match the python function of the same name. Is that correct?
##########
File path: python/iceberg/api/expressions/expressions.py
##########
@@ -106,6 +106,15 @@ def equal(name, value):
def not_equal(name, value):
return UnboundPredicate(Operation.NOT_EQ, Expressions.ref(name), value)
+ # In Python we use "startswith", to match the direct conversion & the pythonic impl do both.
Review comment:
After looking at this for a minute, it does not seem pretty clear to me.
Feel free to edit or close this thread at your discretion and I'm gonna make myself another cup. 🙂
--
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
[GitHub] [iceberg] holdenk commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r679364087
##########
File path: python/iceberg/api/expressions/expression_parser.py
##########
@@ -44,7 +44,7 @@
ident = Word(alphas, alphanums + "_$").setName("identifier")
columnName = delimitedList(ident, ".", combine=True).setName("column name")
-binop = oneOf("= == != < > >= <= eq ne lt le gt ge <>", caseless=False)
+binop = oneOf("= == != < > >= <= eq ne lt le gt ge <> startsWith", caseless=False)
Review comment:
Sure that sounds like a reasonable suggestion
--
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
[GitHub] [iceberg] TGooch44 commented on a change in pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on a change in pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#discussion_r679072644
##########
File path: python/iceberg/api/expressions/inclusive_manifest_evaluator.py
##########
@@ -159,3 +159,7 @@ def in_(self, ref, lit):
def not_in(self, ref, lit):
return ROWS_MIGHT_MATCH
+
+ def starts_with(self, ref, lit):
+ # if it's all null then we can't match :)
+ return self.not_null(ref)
Review comment:
I don't have a strong opinion here but the convention in the java library is that the leaf predicates always return either of those constants.
--
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
[GitHub] [iceberg] holdenk commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-870965038
cc @kbendick who filed the issue & @TGooch44 who implemented NOT_EQ on which I based this for review/feedback. Thank you :)
--
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
[GitHub] [iceberg] holdenk commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-885933174
A little bit of clarity around `ROWS_MUST_MATCH` in `strict_metrics_evaluator.py`, does that mean that all of the rows must match? Because the return from `is_null` doesn't make sense to me there.
--
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
[GitHub] [iceberg] TGooch44 commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
TGooch44 commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-872338036
@holdenk Changes here look good to me thanks for getting this in!
There are a few more evaluators that will need to be implemented for full coverage: `InclusiveManifestEvaluator`, `InclusiveMetricsEvaluator`, and `StrictMetricsEvaluator` additionally for row level filtering in parquet there will need to be a clause in the `predicate` function in `parquet.arrow_table_utils` to handle a `startswith` residual.
--
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
[GitHub] [iceberg] holdenk commented on pull request #2761: Add an implemention of STARTS_WITH for Python API.
Posted by GitBox <gi...@apache.org>.
holdenk commented on pull request #2761:
URL: https://github.com/apache/iceberg/pull/2761#issuecomment-888543857
cc @TGooch44 I addressed your comments if you've got some cycles to review I'd appreciate it :) No rush though :)
--
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