Skip to content
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: add support for overriding context.engine by magic argument #60

Merged
merged 3 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions bigquery_magics/bigquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,15 @@ def _create_dataset_if_necessary(client, dataset_id):
"This flag is ignored when the engine is 'bigframes'."
),
)
@magic_arguments.argument(
"--engine",
type=str,
default=None,
help=(
"Set the execution engine, either 'pandas' or 'bigframes'."
"Defaults to engine set in the query setting in console."
),
)
def _cell_magic(line, query):
"""Underlying function for bigquery cell magic

Expand All @@ -384,7 +393,9 @@ def _cell_magic(line, query):
return
query = _validate_and_resolve_query(query, args)

if context.engine == "bigframes":
use_bigframes_engine = args.engine or context.engine

if use_bigframes_engine == "bigframes":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name feels off to me. Like it would be a boolean but it's not. How about engine_or_default

Suggested change
use_bigframes_engine = args.engine or context.engine
if use_bigframes_engine == "bigframes":
engine_or_default = args.engine or context.engine
if engine_or_default == "bigframes":

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I named it simply"engine"

return _query_with_bigframes(query, params, args)

return _query_with_pandas(query, params, args)
Expand Down Expand Up @@ -423,7 +434,12 @@ def _parse_magic_args(line: str) -> Tuple[List[Any], Any]:

params = _helpers.to_query_parameters(ast.literal_eval(params_option_value), {})

return params, magic_arguments.parse_argstring(_cell_magic, rest_of_args)
args = magic_arguments.parse_argstring(_cell_magic, rest_of_args)

if args.engine is not None and args.engine not in ("pandas", "bigframes"):
raise ValueError(f"Invalid engine: {args.engine}")

return params, args


def _split_args_line(line: str) -> Tuple[str, str]:
Expand Down
62 changes: 54 additions & 8 deletions tests/unit/test_bigquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def mock_credentials(monkeypatch):


@pytest.fixture
def bigframes_engine(monkeypatch):
def set_bigframes_engine_in_context(monkeypatch):
monkeypatch.setattr(bigquery_magics.context, "engine", "bigframes")


Expand Down Expand Up @@ -1896,8 +1896,20 @@ def test_bigquery_magic_with_location():
assert client_options_used.location == "us-east1"


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
def test_big_query_magic_bigframes():
@pytest.mark.usefixtures("ipython_interactive")
def test_bigquery_magic_with_invalid_engine_raises_error():
ip = IPython.get_ipython()
ip.extension_manager.load_extension("bigquery_magics")
engine = "whatever"

with pytest.raises(ValueError, match=f"Invalid engine: {engine}"):
ip.run_cell_magic("bigquery", f"--engine {engine}", "SELECT 17 AS num")


@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes_set_in_context():
if bpd is None:
pytest.skip("BigFrames not installed")

Expand All @@ -1920,7 +1932,33 @@ def test_big_query_magic_bigframes():
assert bpd.options.bigquery.project == bigquery_magics.context.project


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures("ipython_interactive", "mock_credentials")
def test_big_query_magic_bigframes_set_in_args():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: big_query -> bigquery

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Updated the name here and for other methods too.

if bpd is None:
pytest.skip("BigFrames not installed")

ip = IPython.get_ipython()
ip.extension_manager.load_extension("bigquery_magics")
sql = "SELECT 0 AS something"
expected_configuration = {
"query": {"queryParameters": [], "useLegacySql": False},
"dryRun": False,
}
bf_patch = mock.patch("bigframes.pandas.read_gbq_query", autospec=True)

with bf_patch as bf_mock:
ip.run_cell_magic("bigquery", "--engine bigframes", sql)

bf_mock.assert_called_once_with(
sql, max_results=None, configuration=expected_configuration
)
assert bpd.options.bigquery.credentials is bigquery_magics.context.credentials
assert bpd.options.bigquery.project == bigquery_magics.context.project


@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes__bigframes_is_not_installed__should_raise_error():
if bpd is not None:
pytest.skip("BigFrames is installed")
Expand All @@ -1933,7 +1971,9 @@ def test_big_query_magic_bigframes__bigframes_is_not_installed__should_raise_err
ip.run_cell_magic("bigquery", "", sql)


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes_with_params():
if bpd is None:
pytest.skip("BigFrames not installed")
Expand Down Expand Up @@ -1965,7 +2005,9 @@ def test_big_query_magic_bigframes_with_params():
)


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes_with_max_results():
if bpd is None:
pytest.skip("BigFrames not installed")
Expand All @@ -1987,7 +2029,9 @@ def test_big_query_magic_bigframes_with_max_results():
)


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes_with_destination_var(ipython_ns_cleanup):
if bpd is None:
pytest.skip("BigFrames not installed")
Expand All @@ -2007,7 +2051,9 @@ def test_big_query_magic_bigframes_with_destination_var(ipython_ns_cleanup):
assert df is bf_mock.return_value


@pytest.mark.usefixtures("ipython_interactive", "mock_credentials", "bigframes_engine")
@pytest.mark.usefixtures(
"ipython_interactive", "mock_credentials", "set_bigframes_engine_in_context"
)
def test_big_query_magic_bigframes_with_dry_run__should_fail():
if bpd is None:
pytest.skip("BigFrames not installed")
Expand Down