Use custom resolver for query and eval with nested frames. #150
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 14 14
Lines 948 955 +7
=======================================
+ Hits 943 950 +7
Misses 5 5 ☔ View full report in Codecov by Sentry. |
Verify preflighting of nested expressions using AST visitation. Remove logic for splitting queries by string. Now the evaluation is handled by a nested column resolver, and the mixed-mode expressions are preflighted by examining the parsed abstract syntax tree for the query expression.
Also remove `_ensure_spacing`, no longer used.
gitosaurus
force-pushed
the
query-extension
branch
from
29335eb
to
ee57673
Compare
Click here to view all benchmarks. |
This logic was copied from pd.DataFrame.query, and the accompanying comment said that it was to handle an occasional case where `self.loc[b]` would raise an error on a multi-dimensional `b`, but `self[b]` would succeed. I can't cause this error with `.loc` anymore, so the code coverage complains about the unexercised exception clause. Removing it since the limitation that inspired it seems to be gone now.
There was a problem hiding this comment.
Thank you!
I'd ask for more testing and not-implemented error-handling. For example, I'm not sure that assignment with eval is supported: .eval("nested.c = nested.a + nested.b") or .eval("nested.b = x + nested.a").
It looks like it may also close #144 and #146? If yes, could you please add tests for covering those issues?
Yes, this PR does resolve #146, thanks for the tip! #144 is not resolved by this PR, but perhaps this same approach could be extended to cover it in a separate PR. Good catch on the assignment problem. The existing ability for |
Prevent users from depending on the public interface, and also from assuming that a `NestedSeries` has a nest within it, since that is the existing meaning of that prefix.
There was a problem hiding this comment.
Thanks for this @gitosaurus , looks really good! I added a few comments, in addition to the feedback Kostya has provided.
| raise ValueError(msg) | ||
| kwargs["level"] = kwargs.pop("level", 0) + 1 | ||
| kwargs["target"] = None | ||
| # At present, the query expression must be either entirely within the |
There was a problem hiding this comment.
Totally fine to keep this behavior, but if you believe this new implementation would support multi-target queries this could be a restriction we'd lift. Maybe opening a separate issue for this would be best
There was a problem hiding this comment.
I believe this PR makes a significant step toward multi-target queries, but it might be better to make the next step in a separate PR, for simplicity's sake. Will investigate.
Verify preflighting of nested expressions using AST visitation.
Remove logic for splitting queries by string. Now the evaluation is handled by a nested column resolver, and the mixed-mode expressions are preflighted by examining the parsed abstract syntax tree for the query expression.
Resolves #59.
Change Description
Solution Description
Code Quality
Project-Specific Pull Request Checklists
Bug Fix Checklist