You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anxkhn (via GitHub)" <gi...@apache.org> on 2026/07/09 23:19:27 UTC
[PR] fix: correct class name in Long{AboveMax,BelowMin} type-change error [iceberg-python]
anxkhn opened a new pull request, #3634:
URL: https://github.com/apache/iceberg-python/pull/3634
# Rationale for this change
`LongAboveMax.to()` and `LongBelowMin.to()` fall back to
`raise TypeError("Cannot change the type of ...")` when asked to convert to an
unsupported type, but the class name in each message was left as `IntAboveMax` /
`IntBelowMin`, a copy-paste leftover from the `IntAboveMax` / `IntBelowMin`
singletons defined just above. So when a long overflow sentinel is converted to
an unsupported type, the diagnostic misidentifies it as an int literal, which is
misleading when debugging.
For comparison, the neighbouring sentinels already name themselves correctly:
`FloatAboveMax` / `FloatBelowMin`, `IntAboveMax` / `IntBelowMin`. Only the two
`Long*` messages are wrong.
This is the same class of copy-paste-from-the-`Int`-handler slip that
`DecimalLiteral.to(LongType)` had in #3469 (fixed by #3470, which corrected the
returned sentinel *object*); this PR fixes the remaining instance in the
diagnostic *messages* on the `Long*` singletons themselves.
Change: point each message at its own class:
- `pyiceberg/expressions/literals.py` `LongAboveMax.to`: `IntAboveMax` -> `LongAboveMax`
- `pyiceberg/expressions/literals.py` `LongBelowMin.to`: `IntBelowMin` -> `LongBelowMin`
## Are these changes tested?
Yes. Added `test_above_max_long` and `test_below_min_long` in
`tests/expressions/test_literals.py`, mirroring the existing `test_above_max_int`
/ `test_below_min_int` (singleton identity, `str`/`repr`, `value`, the successful
`.to(LongType())` round-trip) and additionally asserting that `.to(IntegerType())`
raises `TypeError` whose message now names the `Long*` class.
Verified red -> green: reverting only the source change makes both new tests fail
with `'Cannot change the type of LongBelowMin' in 'Cannot change the type of
IntBelowMin'`; with the fix, both pass. `tests/expressions/test_literals.py`: 172
passed. `tests/expressions/`: 566 passed, no regressions. `make lint` (ruff,
ruff-format, mypy, pydocstyle, codespell, license/header) passes on the changed
files.
## Are there any user-facing changes?
No API or behavior change. Only the text of an already-raised `TypeError` is
corrected (the exception type and the conditions under which it is raised are
unchanged). No changelog entry needed.
--
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] fix: correct class name in Long{AboveMax,BelowMin} type-change error [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #3634:
URL: https://github.com/apache/iceberg-python/pull/3634
--
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] fix: correct class name in Long{AboveMax,BelowMin} type-change error [iceberg-python]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #3634:
URL: https://github.com/apache/iceberg-python/pull/3634#discussion_r3561740870
##########
tests/expressions/test_literals.py:
##########
@@ -633,6 +633,20 @@ def test_below_min_int() -> None:
assert b.to(IntegerType()) == IntBelowMin()
+def test_above_max_long() -> None:
Review Comment:
```suggestion
def test_long_above_max_to_error() -> None:
```
##########
tests/expressions/test_literals.py:
##########
@@ -633,6 +633,20 @@ def test_below_min_int() -> None:
assert b.to(IntegerType()) == IntBelowMin()
+def test_above_max_long() -> None:
+ a = LongAboveMax()
Review Comment:
We can inline `a`. Same for `LongBelowMin` test.
##########
tests/expressions/test_literals.py:
##########
@@ -633,6 +633,20 @@ def test_below_min_int() -> None:
assert b.to(IntegerType()) == IntBelowMin()
+def test_above_max_long() -> None:
+ a = LongAboveMax()
+ with pytest.raises(TypeError) as e:
+ a.to(IntegerType())
+ assert "Cannot change the type of LongAboveMax" in str(e.value)
+
+
+def test_below_min_long() -> None:
Review Comment:
```suggestion
def test_long_below_min_to_error() -> None:
```
--
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] fix: correct class name in Long{AboveMax,BelowMin} type-change error [iceberg-python]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #3634:
URL: https://github.com/apache/iceberg-python/pull/3634#discussion_r3556315215
##########
tests/expressions/test_literals.py:
##########
@@ -633,6 +633,34 @@ def test_below_min_int() -> None:
assert b.to(IntegerType()) == IntBelowMin()
+def test_above_max_long() -> None:
+ a = LongAboveMax()
+ # singleton
+ assert a == LongAboveMax()
+ assert str(a) == "LongAboveMax"
+ assert repr(a) == "LongAboveMax()"
Review Comment:
What is the motivation of these assertions?
##########
tests/expressions/test_literals.py:
##########
@@ -633,6 +633,34 @@ def test_below_min_int() -> None:
assert b.to(IntegerType()) == IntBelowMin()
+def test_above_max_long() -> None:
+ a = LongAboveMax()
+ # singleton
+ assert a == LongAboveMax()
Review Comment:
Why do we want to verify that `LongAboveMax` is a singleton?
--
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] fix: correct class name in Long{AboveMax,BelowMin} type-change error [iceberg-python]
Posted by "anxkhn (via GitHub)" <gi...@apache.org>.
anxkhn commented on PR #3634:
URL: https://github.com/apache/iceberg-python/pull/3634#issuecomment-5265468468
@ebyhr thanks for the approval. with the requested cleanup complete and checks green, could you merge this?
--
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