Copilot commented on code in PR #9553:
URL: https://github.com/apache/paimon/pull/9553#discussion_r3915539170
##########
paimon-python/pypaimon/multimodal/table.py:
##########
@@ -1135,13 +1142,47 @@ def _coerce_full_text_query(query, method, schema,
column=None):
raise ValueError("%s requires a text string or query mapping." % method)
-def _infer_vector_column(schema: pa.Schema, parameter: str = "vector_column"):
- columns = [
- field.name
+def _resolve_vector_column(
+ schema: pa.Schema,
+ column: Optional[str],
+ dimension: int):
Review Comment:
`_resolve_vector_column()` always returns a column name string (or raises).
Adding an explicit return type annotation (`-> str`) would make the contract
clearer and help type-checkers catch call-site issues.
##########
paimon-python/pypaimon/multimodal/table.py:
##########
@@ -1135,13 +1142,47 @@ def _coerce_full_text_query(query, method, schema,
column=None):
raise ValueError("%s requires a text string or query mapping." % method)
-def _infer_vector_column(schema: pa.Schema, parameter: str = "vector_column"):
- columns = [
- field.name
+def _resolve_vector_column(
+ schema: pa.Schema,
+ column: Optional[str],
+ dimension: int):
+ if column is not None:
+ field = next(
+ (field for field in schema if field.name == column), None)
+ if field is None:
+ raise ValueError(
+ "Vector column '%s' not found in table schema." % column)
+ if not pa.types.is_fixed_size_list(field.type):
+ raise ValueError(
+ "Column '%s' is not a fixed-size vector column." % column)
+ if field.type.list_size != dimension:
+ raise ValueError(
+ "Vector dimension %d does not match column '%s' dimension %d."
+ % (dimension, column, field.type.list_size))
Review Comment:
The mismatch error is accurate but doesn’t indicate what the caller should
do next. Consider extending it with actionable guidance (e.g., mention passing
the correct `column` or using a query vector with the expected dimension) to
reduce support/debug time.
##########
paimon-python/pypaimon/multimodal/table.py:
##########
@@ -1135,13 +1142,47 @@ def _coerce_full_text_query(query, method, schema,
column=None):
raise ValueError("%s requires a text string or query mapping." % method)
-def _infer_vector_column(schema: pa.Schema, parameter: str = "vector_column"):
- columns = [
- field.name
+def _resolve_vector_column(
+ schema: pa.Schema,
+ column: Optional[str],
+ dimension: int):
+ if column is not None:
+ field = next(
+ (field for field in schema if field.name == column), None)
+ if field is None:
+ raise ValueError(
+ "Vector column '%s' not found in table schema." % column)
Review Comment:
Schema field lookup is currently an O(n) scan over `schema`. PyArrow schemas
support indexed/name-based lookup (e.g., `schema.get_field_index(column)` +
`schema.field(idx)`), which avoids repeated linear scans as schema size grows.
Consider switching to a name-index lookup for the explicit `column` path.
--
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]
