-
Notifications
You must be signed in to change notification settings - Fork 609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): add FieldNotFoundError #10412
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,21 +240,39 @@ def _fast_bind(self, *args, **kwargs): | |
args = () | ||
else: | ||
args = util.promote_list(args[0]) | ||
# bind positional arguments | ||
|
||
values = [] | ||
errs = [] | ||
# bind positional arguments | ||
for arg in args: | ||
values.extend(bind(self, arg)) | ||
try: | ||
# need tuple to cause generator to evaluate | ||
bindings = tuple(bind(self, arg)) | ||
except com.FieldNotFoundError as e: | ||
errs.append(e) | ||
continue | ||
values.extend(bindings) | ||
|
||
# bind keyword arguments where each entry can produce only one value | ||
# which is then named with the given key | ||
for key, arg in kwargs.items(): | ||
bindings = tuple(bind(self, arg)) | ||
try: | ||
# need tuple to cause generator to evaluate | ||
bindings = tuple(bind(self, arg)) | ||
except com.FieldNotFoundError as e: | ||
errs.append(e) | ||
continue | ||
if len(bindings) != 1: | ||
raise com.IbisInputError( | ||
"Keyword arguments cannot produce more than one value" | ||
) | ||
(value,) = bindings | ||
values.append(value.name(key)) | ||
if errs: | ||
raise com.IbisError( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if IbisError is the best type for this. |
||
"Error binding arguments to table expression: " | ||
+ "; ".join(str(e) for e in errs) | ||
) | ||
return values | ||
|
||
def bind(self, *args: Any, **kwargs: Any) -> tuple[Value, ...]: | ||
|
@@ -739,8 +757,9 @@ def __getattr__(self, key: str) -> ir.Column: | |
""" | ||
try: | ||
return ops.Field(self, key).to_expr() | ||
except com.IbisTypeError: | ||
pass | ||
except com.FieldNotFoundError as e: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a slight difference in ux here that I'd love to unify
I'm not sure which is better. If we say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also a difference between Tables and Structs. I'd love for them to have the same behavior too. |
||
if e.typos: | ||
raise e | ||
|
||
# A mapping of common attribute typos, mapping them to the proper name | ||
common_typos = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ | |
import ibis.expr.operations as ops | ||
from ibis import util | ||
from ibis.common.deferred import deferrable | ||
from ibis.common.exceptions import IbisError | ||
from ibis.common.exceptions import FieldNotFoundError, IbisError | ||
from ibis.expr.types.generic import Column, Scalar, Value, literal | ||
|
||
if TYPE_CHECKING: | ||
|
@@ -202,10 +202,10 @@ def __getitem__(self, name: str) -> ir.Value: | |
>>> t.s["foo_bar"] | ||
Traceback (most recent call last): | ||
... | ||
KeyError: 'foo_bar' | ||
ibis.common.exceptions.FieldNotFoundError: 'foo_bar' not found in StructColumn object. Possible options: {'a', 'b'} | ||
""" | ||
if name not in self.names: | ||
raise KeyError(name) | ||
raise FieldNotFoundError(self, name, self.names) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to be breaking here and not raise a KeyError anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. KeyError seems semantically a little wrong. I think KeyError should be for collections with a dynamic set of keys, such as a vanilla python |
||
return ops.StructField(self, name).to_expr() | ||
|
||
def __setstate__(self, instance_dictionary): | ||
|
@@ -262,12 +262,9 @@ def __getattr__(self, name: str) -> ir.Value: | |
>>> t.s.foo_bar | ||
Traceback (most recent call last): | ||
... | ||
AttributeError: foo_bar | ||
ibis.common.exceptions.FieldNotFoundError: 'foo_bar' not found in StructColumn object. Possible options: {'a', 'b'} | ||
""" | ||
try: | ||
return self[name] | ||
except KeyError: | ||
raise AttributeError(name) from None | ||
return self[name] | ||
|
||
@property | ||
def names(self) -> Sequence[str]: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat orthogonal to the FieldNotFoundError stuff. But if I have multiple bogus columns, eg
table.select("bogus", "also_bogus")
I only see the first error. I want to see them all.