-
Notifications
You must be signed in to change notification settings - Fork 149
SNOW-3484790: initialize aggregation functions list during SCOS init #4217
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
Merged
+531
−83
Merged
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
92b4d70
use local list instead of fetch from snowflake
sfc-gh-yuwang 937650a
test
sfc-gh-yuwang 1650ef6
aysnc update
sfc-gh-yuwang 09b3946
add test
sfc-gh-yuwang c606f3f
update change
sfc-gh-yuwang 971ce2e
address comment
sfc-gh-yuwang b49e48f
add lock for async job object
sfc-gh-yuwang 73ee4e0
make waiting threads reuse async job result instead of issuing redund…
sfc-gh-yuwang a47adc6
fix event-before-publish race and add thread-safety tests
sfc-gh-yuwang d3dc487
use RLock, inline prefetch helper, clean up tests
sfc-gh-yuwang abaae63
fix lint
sfc-gh-yuwang ad2d18e
remove functions with (*)
sfc-gh-yuwang 85c4c9f
Merge branch 'main' into SNOW-3484790
sfc-gh-yuwang 90b836c
fix logic
sfc-gh-yuwang b951239
add lock to protect
sfc-gh-yuwang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
| from collections import defaultdict | ||
| from functools import reduce | ||
| from logging import getLogger | ||
| from threading import RLock | ||
| from threading import Event, Lock, RLock | ||
| from types import ModuleType | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
|
|
@@ -856,8 +856,15 @@ def __init__( | |
| self._dataframe_profiler = DataframeProfiler(session=self) | ||
| self._catalog = None | ||
| self._client_telemetry = EventTableTelemetry(session=self) | ||
| self._agg_function_prefetch_job: Optional[AsyncJob] = None | ||
| # Guards the one-time atomic claim of _agg_function_prefetch_job. | ||
| self._agg_function_prefetch_lock = Lock() | ||
| # Set by the thread that claimed the async job once it finishes (success or failure), | ||
| # so other threads can wait instead of issuing redundant sync queries. | ||
| self._agg_function_fetch_event: Optional[Event] = None | ||
|
|
||
| self._ast_batch = AstBatch(self) | ||
| self._start_async_aggregation_prefetch_if_needed() | ||
|
|
||
| _logger.info("Snowpark Session information: %s", self._session_info) | ||
|
|
||
|
|
@@ -5055,43 +5062,139 @@ def _retrieve_aggregation_function_list(self) -> None: | |
| return | ||
|
|
||
| retrieved_set = set() | ||
| system_fetch_succeeded = False | ||
|
|
||
| # Atomically claim the async job. The claiming thread creates an Event so concurrent | ||
| # threads can wait on it rather than issuing redundant sync queries. | ||
| # AsyncJob.result() is not thread-safe — the underlying connector cursor mutates | ||
| # shared state (_result, _rownumber, _prefetch_hook) during result fetching, so only | ||
| # one thread may call it. The lock is held only for the pointer swap and event setup | ||
| # (nanoseconds), not the network call itself. | ||
| with self._agg_function_prefetch_lock: | ||
| job, self._agg_function_prefetch_job = self._agg_function_prefetch_job, None | ||
| if job is not None: | ||
| fetch_event = Event() | ||
| self._agg_function_fetch_event = fetch_event | ||
| wait_event = None | ||
| elif self._agg_function_fetch_event is not None: | ||
| fetch_event = None | ||
| wait_event = self._agg_function_fetch_event | ||
| else: | ||
| fetch_event = None | ||
| wait_event = None | ||
|
|
||
| if wait_event is not None: | ||
| # The query typically finishes in ~5s; 20s gives ample headroom while | ||
| # bounding the hang in the rare case the winner thread dies before its | ||
| # finally block runs (e.g. os._exit, interpreter shutdown). | ||
| wait_event.wait(timeout=20) | ||
| if context._aggregation_function_set: | ||
|
sfc-gh-bkogan marked this conversation as resolved.
Outdated
|
||
| return | ||
| # Winner failed or timed out; fall through to sync query. | ||
|
|
||
| # User-defined aggregation functions | ||
| try: | ||
| retrieved_set.update( | ||
| { | ||
| r[0].lower() | ||
| for r in self.sql( | ||
| """select function_name from information_schema.functions where is_aggregate = 'YES'""" | ||
| ).collect() | ||
| } | ||
| ) | ||
| except Exception as e: | ||
| _logger.debug( | ||
| "Unable to get user-defined aggregation functions: %s", | ||
| e, | ||
| ) | ||
| if job is not None: | ||
| try: | ||
| retrieved_set.update( | ||
| {r[0].lower() for r in job.result()} | ||
| ) | ||
| system_fetch_succeeded = True | ||
| except Exception as e: | ||
| _logger.debug( | ||
| "Unable to use async aggregation function prefetch: %s", | ||
| e, | ||
| ) | ||
| else: | ||
| _logger.debug( | ||
| "Async aggregation function prefetch job is unavailable; using sync fallback." | ||
| ) | ||
| try: | ||
| retrieved_set.update( | ||
| { | ||
| r[0].lower() | ||
| for r in self._conn.run_query( | ||
| """show functions ->> select "name" from $1 where "is_aggregate" = 'Y' | ||
| union | ||
| select function_name from information_schema.functions where is_aggregate = 'YES'""", | ||
| _is_internal=True, | ||
| )["data"] | ||
| } | ||
| ) | ||
| system_fetch_succeeded = True | ||
| except Exception as e: | ||
| _logger.debug( | ||
| "Unable to get aggregation functions via sync union query: %s", | ||
| e, | ||
| ) | ||
|
|
||
| # Sync fallback query. | ||
| if not system_fetch_succeeded: | ||
| try: | ||
| retrieved_set.update( | ||
| { | ||
| r[0].lower() | ||
| for r in self._conn.run_query( | ||
| """show functions ->> select "name" from $1 where "is_aggregate" = 'Y'""", | ||
| _is_internal=True, | ||
| )["data"] | ||
| } | ||
| ) | ||
| system_fetch_succeeded = True | ||
| except Exception as e: | ||
| _logger.debug( | ||
| "Unable to get aggregation functions via sync fallback query: %s", | ||
| e, | ||
| ) | ||
|
|
||
| # Fallback to the local hardcoded list only when metadata retrieval fails. | ||
| if not system_fetch_succeeded: | ||
| retrieved_set.update(context._KNOWN_AGGREGATION_FUNCTIONS) | ||
|
sfc-gh-bkogan marked this conversation as resolved.
|
||
|
|
||
| with context._aggregation_function_set_lock: | ||
| context._aggregation_function_set.update(retrieved_set) | ||
| finally: | ||
| # Signal after _aggregation_function_set is published so waiters see | ||
| # the populated set immediately upon waking. Also fires on BaseException | ||
| # (e.g. KeyboardInterrupt) so waiters are never left blocking until timeout. | ||
| if fetch_event is not None: | ||
| fetch_event.set() | ||
|
|
||
| def _start_async_aggregation_prefetch_if_needed(self) -> None: | ||
| """Start aggregation metadata prefetch only when not already in progress.""" | ||
| if not ( | ||
| context._is_snowpark_connect_compatible_mode | ||
| and context._snowpark_connect_flatten_select_after_sort | ||
| ): | ||
| return | ||
| if context._aggregation_function_set: | ||
| return | ||
| if self._agg_function_prefetch_job is not None: | ||
| return | ||
|
|
||
| # System built-in aggregation functions | ||
| try: | ||
| retrieved_set.update( | ||
| { | ||
| r[0].lower() | ||
| for r in self.sql( | ||
| """show functions ->> select "name" from $1 where "is_aggregate" = 'Y'""" | ||
|
sfc-gh-joshi marked this conversation as resolved.
|
||
| ).collect() | ||
| } | ||
| self._agg_function_prefetch_job = self._submit_internal_async_prefetch_query( | ||
|
sfc-gh-joshi marked this conversation as resolved.
Outdated
|
||
| """show functions ->> select "name" from $1 where "is_aggregate" = 'Y' | ||
| union | ||
| select function_name from information_schema.functions where is_aggregate = 'YES'""" | ||
| ) | ||
| except Exception as e: | ||
| except Exception as e: # pragma: no cover | ||
| _logger.debug( | ||
| "Unable to get system aggregation functions, " | ||
| "falling back to hardcoded list: %s", | ||
| "Unable to start async aggregation metadata prefetch: %s", | ||
| e, | ||
| ) | ||
| retrieved_set.update(context._KNOWN_AGGREGATION_FUNCTIONS) | ||
| self._agg_function_prefetch_job = None | ||
|
|
||
| with context._aggregation_function_set_lock: | ||
| context._aggregation_function_set.update(retrieved_set) | ||
| def _submit_internal_async_prefetch_query(self, query: str) -> Optional[AsyncJob]: | ||
|
Contributor
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. nit: Can we inline this method since it's only called once, and pretty short? |
||
| """Submit a prefetch query as internal async and return an AsyncJob handle.""" | ||
| try: | ||
| result = self._conn.execute_async_and_notify_query_listener( | ||
| query, | ||
| _is_internal=True, | ||
| ) | ||
| return self.create_async_job(result["queryId"]) | ||
| except Exception as e: # pragma: no cover | ||
| _logger.debug("Unable to submit internal async prefetch query: %s", e) | ||
| return None | ||
|
|
||
| def directory(self, stage_name: str, _emit_ast: bool = True) -> DataFrame: | ||
| """ | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't think there's a way for the same thread to attempt to acquire this lock multiple times, but I think we should make this an
RLockinstead (which is already imported in this file) to be safe.