Copilot commented on code in PR #973:
URL: https://github.com/apache/sedona-db/pull/973#discussion_r3431396192
##########
python/sedonadb/python/sedonadb/expr/expression.py:
##########
@@ -328,6 +328,23 @@ def __len__(self) -> int:
"the Expr against a frame (e.g. `df.filter(expr).count()`)."
)
+ # Nested expressions
+ def __getitem__(self, key: Union[int, str, "Expr"]) -> "Expr":
+ if isinstance(key, int):
+ # Python uses 0-based indexing; SQL uses 1-based indexing
+ return self.funcs.array_extract(key + 1)
+ elif isinstance(key, str):
+ # get_field works for both structs and maps, returning a scalar
+ return self.funcs.get_field(key)
+ else:
+ raise ValueError(
+ "Expr keys are not yet supported. Use .funcs.array_extract() "
+ "or .funcs.get_field() to extract with an expression key."
+ )
Review Comment:
The type signature advertises support for `key: Expr` (`Union[int, str,
\"Expr\"]`), but the implementation raises for non-`int`/`str`. Either
implement expression keys (e.g., for map/array lookups driven by another
expression) or remove `\"Expr\"` from the accepted types and make the error
message consistent with the supported inputs.
##########
python/sedonadb/python/sedonadb/expr/expression.py:
##########
@@ -328,6 +328,23 @@ def __len__(self) -> int:
"the Expr against a frame (e.g. `df.filter(expr).count()`)."
)
+ # Nested expressions
+ def __getitem__(self, key: Union[int, str, "Expr"]) -> "Expr":
+ if isinstance(key, int):
+ # Python uses 0-based indexing; SQL uses 1-based indexing
+ return self.funcs.array_extract(key + 1)
+ elif isinstance(key, str):
+ # get_field works for both structs and maps, returning a scalar
+ return self.funcs.get_field(key)
+ else:
+ raise ValueError(
+ "Expr keys are not yet supported. Use .funcs.array_extract() "
+ "or .funcs.get_field() to extract with an expression key."
+ )
+
+ def __getattr__(self, name: str) -> "Expr":
+ return self.funcs.get_field(name)
Review Comment:
`__getattr__` will turn any missing attribute access (including typos like
`exrpr`) into a nested-field expression instead of raising `AttributeError`,
which can hide real bugs. Consider guarding against common non-field attribute
patterns (e.g., names starting with `_`) and/or offering an explicit API (like
`expr.field(\"a\")`) while keeping attribute access more conservative.
##########
python/sedonadb/python/sedonadb/expr/expression.py:
##########
@@ -328,6 +328,23 @@ def __len__(self) -> int:
"the Expr against a frame (e.g. `df.filter(expr).count()`)."
)
+ # Nested expressions
+ def __getitem__(self, key: Union[int, str, "Expr"]) -> "Expr":
+ if isinstance(key, int):
+ # Python uses 0-based indexing; SQL uses 1-based indexing
+ return self.funcs.array_extract(key + 1)
Review Comment:
Negative indices currently translate to `array_extract(0)` (or other
non-positive indices), which is not valid under typical 1-based SQL indexing
semantics and will likely yield incorrect results or errors. Consider
explicitly rejecting negative indices with an `IndexError`/`ValueError`, or (if
desired) defining supported behavior for Python-style negative indexing.
--
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]
