perf: Directly read gbq table for simple plans by TrevorBergeron · Pull Request #1607 · googleapis/python-bigquery-dataframes

TrevorBergeron · 2025-04-09T21:34:31Z

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:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

tswast · 2025-04-10T19:06:43Z

bigframes/core/nodes.py

@@ -574,6 +583,31 @@ def with_id(self, id: identifiers.ColumnId) -> ScanItem:
 class ScanList:


Could we get a docstring for ScanList, please?

tswast · 2025-04-10T19:08:11Z

bigframes/core/nodes.py

 class ScanList:
    items: typing.Tuple[ScanItem, ...]

+    def filter(


Should we call this project to align with https://en.wikipedia.org/wiki/Projection_(relational_algebra) ?

renamed to project

actually renamed this one to filter_cols

tswast · 2025-04-10T19:09:53Z

bigframes/core/nodes.py

+    ) -> ScanList:
+        result = ScanList(tuple(item for item in self.items if item.id in ids))
+        if len(result.items) == 0:
+            # We need to select something, or stuff breaks


Ideally we'd pick the smallest size column, right? Let's add a TODO if so. Or does this column not actually get scanned?

Would be good to describe more what stuff breaks, too.

specified that its the sql syntax that breaks. The columns won't actually get scanned, as it will just be pruned out by the optimizer anyways. Amended commend to say that its the sql syntax

tswast · 2025-04-10T19:11:05Z

bigframes/core/nodes.py

+            result = ScanList(self.items[:1])
+        return result
+
+    def select(


A docstring would be great. Are we talking about https://en.wikipedia.org/wiki/Selection_(relational_algebra) (aka row predicates) here?

tswast · 2025-04-10T19:20:56Z

bigframes/session/read_api_execution.py

@@ -0,0 +1,81 @@
+from typing import Any, Optional


Needs license header.

Suggested change

from typing import Any, Optional

# Copyright 2025 Google LLC

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

from __future__ import annotations

from typing import Any, Optional

tswast · 2025-04-10T19:21:29Z

bigframes/session/read_api_execution.py

+from bigframes.session import executor, semi_executor
+
+
+class ReadApiSemiExecutor(semi_executor.SemiExecutor):


A docstring would be great. I assume the "Semi" part means that not all plans/expressions are supported?

Added docstring to SemiExecutor abc, as well as ReadApiSemiExecutor. And yeah, SemiExecutor means exactly that, they can execute a subset of possible plans.

tswast · 2025-04-10T19:24:17Z

bigframes/session/read_api_execution.py

+        if node.source.sql_predicate:
+            read_options["row_restriction"] = node.source.sql_predicate


Aside: How are we protecting against plans that we can't run. Hopefully we've got an allowlist of properties. I'm worried about adding a property to BigFrameNode and forgetting to add it to the executor.

Ultimately, each semi-executor has to specify the subset of plans it can or cannot execute, whether through allowlists, if-else statements, or some other mechanism. The real problem is if it drifts out of sync with the expected reference behavior. This shouldn't be a problem for now, as all tests that can use this semi-executor, will, and will catch issues. However, as we add more semi-executors, we should directly compare the results against the reference implementation, and maybe even do some fuzzing.

tswast · 2025-04-10T19:26:01Z

bigframes/session/read_api_execution.py

+            return None
+
+        import google.cloud.bigquery_storage_v1.types as bq_storage_types
+        from google.protobuf.timestamp_pb2 import Timestamp


Nit: Use import statements for packages and modules only, not for individual types, classes, or functions.

https://google.github.io/styleguide/pyguide.html#22-imports

Timestamp isn't in the list of excemptions. https://google.github.io/styleguide/pyguide.html#2241-exemptions

sycai · 2025-04-15T17:26:03Z

bigframes/core/nodes.py

        return 0

+    @property
+    def double_references_ids(self) -> bool:


naming nit: maybe "has_multi_reference_ids" is better, since the return type is bool?

Plus, technically speaking you can have triple references too 🧐

sycai · 2025-04-15T17:28:55Z

bigframes/core/rewrite/scan_reduction.py

+
+
+def try_reduce_to_table_scan(node: nodes.BigFrameNode) -> Optional[nodes.ReadTableNode]:
+    if not all(


Shall we re-write this into a plain-old "for" loop?

for n in node.unique_nodes(): if not isinstance(n, ...): return None

This might be more readable.

yeah, makes sense, I tend too much towards "everything is an expression". fixed

sycai · 2025-04-15T17:32:15Z

bigframes/session/semi_executor.py

+# Unstable interface, in development
+class SemiExecutor(abc.ABC):
+    """
+    A semi executor executes a subset of possible plans, returns None if it cannot execute a given plan.


"returns None if it cannot execute a given plan."

Maybe it should be "... if cannot execute any/all given plans?"

Rephrased, but it is give a single (logical) plan at a time, and if it cannot find a way to physically execute it, it returns None.

product-auto-label bot added size: xl Pull request size is extra large. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Apr 9, 2025

TrevorBergeron force-pushed the semi_executor branch from 3a9dc4b to 84c4aa0 Compare April 9, 2025 21:37

perf: Directly read gbq table for simple plans

83312f0

TrevorBergeron force-pushed the semi_executor branch from 84c4aa0 to 83312f0 Compare April 9, 2025 22:03

TrevorBergeron requested a review from tswast April 9, 2025 22:03

TrevorBergeron marked this pull request as ready for review April 9, 2025 22:03

TrevorBergeron requested review from a team as code owners April 9, 2025 22:03

blunderbuss-gcf bot assigned jiaxunwu Apr 9, 2025

tswast reviewed Apr 10, 2025

View reviewed changes

fixes

5785954

TrevorBergeron requested a review from tswast April 13, 2025 05:35

TrevorBergeron added 2 commits April 14, 2025 23:15

Merge remote-tracking branch 'github/main' into semi_executor

3785657

fix merge issue

669cc6f

TrevorBergeron requested a review from sycai April 15, 2025 17:16

sycai approved these changes Apr 15, 2025

View reviewed changes

readability improvements

d4259f4

TrevorBergeron enabled auto-merge (squash) April 15, 2025 19:00

TrevorBergeron merged commit 6ad38e8 into main Apr 15, 2025
17 of 24 checks passed

TrevorBergeron deleted the semi_executor branch April 15, 2025 19:25

release-please bot mentioned this pull request Apr 15, 2025

chore(main): release 2.0.0 #1552

Merged

		@@ -574,6 +583,31 @@ def with_id(self, id: identifiers.ColumnId) -> ScanItem:
		class ScanList:

-from typing import Any, Optional
+# Copyright 2025 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+from __future__ import annotations
+from typing import Any, Optional

		from bigframes.session import executor, semi_executor


		class ReadApiSemiExecutor(semi_executor.SemiExecutor):

		if node.source.sql_predicate:
		read_options["row_restriction"] = node.source.sql_predicate



		def try_reduce_to_table_scan(node: nodes.BigFrameNode) -> Optional[nodes.ReadTableNode]:
		if not all(

Conversation

TrevorBergeron commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants