Copilot commented on code in PR #935:
URL: https://github.com/apache/sedona-db/pull/935#discussion_r3384183544
##########
python/sedonadb/python/sedonadb/context.py:
##########
@@ -290,13 +292,13 @@ def read_parquet(
return DataFrame(
self,
self._impl.read_parquet(
- [str(path) for path in table_paths],
+ table_paths_list,
options,
geometry_columns,
validate,
None if partitioning is None else list(partitioning),
),
- )
+ )._ensure_aliased(table_paths)
Review Comment:
`read_parquet()` computes `table_paths_list` by consuming `table_paths` when
it's an iterable, but then uses the original `table_paths` object to derive the
default alias. For generators (e.g. `Path.glob(...)`) this yields a
non-meaningful alias like `generator_<id>` and can even reference an exhausted
iterator; it also won't match the intention/tests of aliasing based on the file
being read.
##########
python/sedonadb/python/sedonadb/context.py:
##########
@@ -435,11 +441,11 @@ def read_format(
self,
self._impl.read_external_format(
spec,
- [str(path) for path in table_paths],
+ table_paths_list,
check_extension,
None if partitioning is None else list(partitioning),
),
- )
+ )._ensure_aliased(table_paths)
Review Comment:
`read_format()` defines `table_paths_list` only in the `else` branch; for a
single `str/Path` input this will raise `NameError` when calling
`_impl.read_external_format(...)`. Also, the alias source should be derived
from the resolved string path(s), not the original iterable (which may be
exhausted).
##########
python/sedonadb/python/sedonadb/context.py:
##########
@@ -378,11 +382,11 @@ def read_pyogrio(
self,
self._impl.read_external_format(
spec,
- [str(path) for path in table_paths],
+ table_paths_list,
False,
None if partitioning is None else list(partitioning),
),
- )
+ )._ensure_aliased(table_paths)
Review Comment:
`read_pyogrio()` computes `table_paths_list` by consuming `table_paths` when
it's an iterable, but then uses the original `table_paths` object to derive the
default alias. For iterators (e.g. `Path.glob(...)`) this produces an unhelpful
alias based on the iterator type/id rather than the file path(s).
##########
python/sedonadb/python/sedonadb/dataframe.py:
##########
@@ -1429,6 +1429,18 @@ def _out_width(self, width=None) -> int:
return width
+ def _ensure_aliased(self, src: Any) -> "DataFrame":
+ return self.alias(_default_alias_for_obj(src))
+
+
+def _default_alias_for_obj(obj: Any) -> str:
+ if isinstance(obj, str):
+ return obj
+ elif isinstance(obj, Path):
+ return str(obj)
+
+ return f"{type(obj).__name__}_{id(obj)}"
Review Comment:
`_default_alias_for_obj()` returns the full input string/path verbatim for
`str`/`Path`, which (a) is often not very meaningful (full URL / full directory
path) and (b) is not even "probably unique" (reading the same path twice yields
identical aliases and can still cause ambiguous references in joins). Consider
deriving the alias from the basename and adding a per-DataFrame suffix to avoid
collisions.
--
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]
