-
Notifications
You must be signed in to change notification settings - Fork 49
test: Add ReadLocalNode tests #1794
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
base: main
Are you sure you want to change the base?
Conversation
205b9c5
to
f43f938
Compare
f43f938
to
cd51e02
Compare
6ff46da
to
9c87dbd
Compare
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.
So cool!
# used only in testing right now, BigQueryCachingExecutor is the fully featured engine | ||
# simplified, doest not do large >10 gb result queries, error handling, respect global config | ||
# or record metrics | ||
class DirectGbqExecutor(semi_executor.SemiExecutor): |
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.
I assume the purpose is to make sure caching and such don't cause any regressions / differences in behavior? Might be good to include that in the comment / docstring if so.
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.
yeah, mostly just to isolate the simplest, fastest version of bq execution, and avoid slow/complicated stuff only needed at >10gb scale or for stateful interactive flows. added comment in new revision
# This will error out if polars is not installed | ||
from bigframes.core.compile.polars import PolarsCompiler |
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.
Might be good to add a little helper to make sure folks know what package (and possibly versions) to install. See pandas helpers like this:
Or closer to home, the versions helpers in some of our client libraries:
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.
hmm, thats a good idea. I am going to expose polars execution in another PR soon, so I'll figure out the messaging experience in that one.
from bigframes.testing import polars_session | ||
|
||
session = polars_session.TestSession() | ||
with bigframes.core.global_session._GlobalSessionContext(session): |
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.
Neat! Potentially something we want to consider exposing to users? I guess after someone asks for it so as not to pollute our public surface.
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.
Maybe if we get some requests, can productionize it. I'm worried its not robust to all the thread-local stuff however?
|
||
pytest.importorskip("polars") | ||
|
||
REFERENCE_ENGINE = polars_executor.PolarsExecutor() |
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.
I worry about this REFERENCE_ENGINE name. Might be worth a comment that the BigQuery engine is the source of truth, but we use this as the reference for faster testing?
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.
comment added
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕