You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2025/08/07 14:15:39 UTC
[PR] Enable add tests migrated Hive tables [iceberg-python]
Fokko opened a new pull request, #2295:
URL: https://github.com/apache/iceberg-python/pull/2295
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace ${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
# Are these changes tested?
# Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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] Enable add tests migrated Hive tables [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #2295:
URL: https://github.com/apache/iceberg-python/pull/2295
--
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] Enable add tests migrated Hive tables [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2295:
URL: https://github.com/apache/iceberg-python/pull/2295#discussion_r2260614541
##########
pyiceberg/expressions/visitors.py:
##########
@@ -940,7 +936,7 @@ def visit_bound_predicate(self, predicate: BoundPredicate[L]) -> BooleanExpressi
def translate_column_names(
- expr: BooleanExpression, file_schema: Schema, case_sensitive: bool, projected_field_values: Dict[int, Any] = EMPTY_DICT
+ expr: BooleanExpression, file_schema: Schema, case_sensitive: bool = True, projected_field_values: Dict[int, Any] = EMPTY_DICT
Review Comment:
👍 case_sensitive is default to True in other places as well
https://grep.app/search?f.repo=apache%2Ficeberg-python&f.repo.pattern=iceberg-python&q=case_sensitive%3A
##########
tests/expressions/test_visitors.py:
##########
@@ -1891,15 +1881,14 @@ def test_translate_column_names_missing_column_projected_field_fallbacks_to_init
)
# Projected field value that differs from both the expression literal and initial_default
- projected_field_values = {2: 10} # This doesn't match expression literal (42)
-
- # Translate column names
translated_expr = translate_column_names(
- bound_expr, file_schema, case_sensitive=True, projected_field_values=projected_field_values
+ bound_expr,
+ file_schema,
+ projected_field_values={2: 10}, # This doesn't match expression literal (42)
)
- # Should evaluate to AlwaysTrue since projected field value doesn't match but initial_default does
- assert translated_expr == AlwaysTrue()
+ # Should evaluate to AlwaysFalse since projected field value doesn't
Review Comment:
nit can we also change the test name?
its currently `test_translate_column_names_missing_column_projected_field_fallbacks_to_initial_default` which is a bit confusing
how about `test_translate_column_names_missing_column_projected_field_ignores_initial_default`?
##########
tests/expressions/test_visitors.py:
##########
@@ -1891,15 +1881,14 @@ def test_translate_column_names_missing_column_projected_field_fallbacks_to_init
)
# Projected field value that differs from both the expression literal and initial_default
- projected_field_values = {2: 10} # This doesn't match expression literal (42)
-
- # Translate column names
translated_expr = translate_column_names(
- bound_expr, file_schema, case_sensitive=True, projected_field_values=projected_field_values
+ bound_expr,
+ file_schema,
+ projected_field_values={2: 10}, # This doesn't match expression literal (42)
)
- # Should evaluate to AlwaysTrue since projected field value doesn't match but initial_default does
- assert translated_expr == AlwaysTrue()
+ # Should evaluate to AlwaysFalse since projected field value doesn't
Review Comment:
```suggestion
# Should evaluate to AlwaysFalse since projected field value doesn't match the expression literal
```
##########
tests/expressions/test_visitors.py:
##########
@@ -1891,15 +1881,14 @@ def test_translate_column_names_missing_column_projected_field_fallbacks_to_init
)
# Projected field value that differs from both the expression literal and initial_default
- projected_field_values = {2: 10} # This doesn't match expression literal (42)
-
- # Translate column names
translated_expr = translate_column_names(
- bound_expr, file_schema, case_sensitive=True, projected_field_values=projected_field_values
+ bound_expr,
+ file_schema,
+ projected_field_values={2: 10}, # This doesn't match expression literal (42)
)
- # Should evaluate to AlwaysTrue since projected field value doesn't match but initial_default does
- assert translated_expr == AlwaysTrue()
+ # Should evaluate to AlwaysFalse since projected field value doesn't
+ assert translated_expr == AlwaysFalse()
def test_translate_column_names_missing_column_projected_field_matches_initial_default_mismatch() -> None:
Review Comment:
i think we can remove this test. its testing the same as the one above. the only difference is the `initial_default` value the `missing_col` field. Its set to 10 here and 42 in the test above.
--
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