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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
