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

SNOW-1663726 make session config updates thread safe #2302

Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
72 commits
Select commit Hold shift + click to select a range
56fb566
init
sfc-gh-aalam Sep 11, 2024
66003d1
make udf/sproc related files thread-safe
sfc-gh-aalam Sep 11, 2024
0e58205
Merge branch 'main' into aalam-SNOW-1418523-make-udf-sproc-thread-safe
sfc-gh-aalam Sep 11, 2024
e75dde1
init
sfc-gh-aalam Sep 11, 2024
68a8c1c
make query listener thread-safe
sfc-gh-aalam Sep 11, 2024
31a5734
Fix query_tag and last_action_id
sfc-gh-aalam Sep 11, 2024
b4dadda
core updates done
sfc-gh-aalam Sep 11, 2024
b8c6496
Add tests
sfc-gh-aalam Sep 12, 2024
f39837e
Fix local tests
sfc-gh-aalam Sep 12, 2024
31a196f
Merge branch 'main' into aalam-SNOW-1418523-make-analyzer-server_conn…
sfc-gh-aalam Sep 12, 2024
723bdf7
Merge branch 'aalam-SNOW-1418523-make-internal-session-variables-thre…
sfc-gh-aalam Sep 12, 2024
37c0419
add file IO tests
sfc-gh-aalam Sep 12, 2024
8a2d433
Merge branch 'aalam-SNOW-1418523-concurrent-file-operations' into aal…
sfc-gh-aalam Sep 12, 2024
a083989
make session._runtime_version_from_requirement safe
sfc-gh-aalam Sep 13, 2024
947d384
add sp/udf concurrent tests
sfc-gh-aalam Sep 13, 2024
fd51720
fix broken test
sfc-gh-aalam Sep 13, 2024
3077853
add udtf/udaf tests
sfc-gh-aalam Sep 13, 2024
65c3186
fix broken test
sfc-gh-aalam Sep 13, 2024
94412cf
sql_simplifier, cte_optimization, eliminate_numeric, query_compilatio…
sfc-gh-aalam Sep 13, 2024
638dd09
cover more configs
sfc-gh-aalam Sep 17, 2024
7ae2c33
fix SnowflakePlan copy
sfc-gh-aalam Sep 17, 2024
1689ebf
minor update
sfc-gh-aalam Sep 17, 2024
5e8a2d2
add description
sfc-gh-aalam Sep 17, 2024
1c83ef2
use _package_lock to protect Session._packages
sfc-gh-aalam Sep 17, 2024
a649761
undo refactor
sfc-gh-aalam Sep 17, 2024
f03d618
undo refactor
sfc-gh-aalam Sep 17, 2024
5f398d5
fix test
sfc-gh-aalam Sep 17, 2024
3807087
fix test
sfc-gh-aalam Sep 17, 2024
4eef3e9
Merge branch 'aalam-SNOW-1418523-make-internal-session-variables-thre…
sfc-gh-aalam Sep 17, 2024
df3263c
add file IO tests
sfc-gh-aalam Sep 12, 2024
6769c54
merge with base
sfc-gh-aalam Sep 17, 2024
af86f67
merge with base
sfc-gh-aalam Sep 17, 2024
a737f33
fix test
sfc-gh-aalam Sep 17, 2024
8ca2730
protect complexity bounds setter with lock
sfc-gh-aalam Sep 17, 2024
81417a3
add config context
sfc-gh-aalam Sep 19, 2024
e340567
add tests
sfc-gh-aalam Sep 19, 2024
30952bb
update documentation
sfc-gh-aalam Sep 20, 2024
03f25b5
use config context in plan compiler
sfc-gh-aalam Sep 20, 2024
6deb402
add comments
sfc-gh-aalam Sep 20, 2024
8e1dfe0
minor refactor
sfc-gh-aalam Sep 20, 2024
10bfeb4
fix test
sfc-gh-aalam Sep 20, 2024
879940a
update documentation
sfc-gh-aalam Sep 20, 2024
5aad2d9
simplify context config
sfc-gh-aalam Sep 25, 2024
669eb91
merge with base
sfc-gh-aalam Sep 25, 2024
a85a144
add config context to repeated subquery elimination resolution stage
sfc-gh-aalam Sep 25, 2024
a79ffb4
fix tests
sfc-gh-aalam Sep 26, 2024
4420350
refactor
sfc-gh-aalam Sep 26, 2024
5f1eaa6
remove do_analyze
sfc-gh-aalam Sep 27, 2024
9d62017
fix
sfc-gh-aalam Sep 27, 2024
b58aa8b
fix
sfc-gh-aalam Sep 27, 2024
db37033
fix
sfc-gh-aalam Sep 27, 2024
dddd15f
fix unit tests
sfc-gh-aalam Sep 27, 2024
57ee9e8
simplify
sfc-gh-aalam Sep 27, 2024
809a86e
simplify
sfc-gh-aalam Sep 27, 2024
6021ab8
simplify
sfc-gh-aalam Sep 27, 2024
43986f6
simplify
sfc-gh-aalam Sep 27, 2024
0430e92
simplify
sfc-gh-aalam Sep 27, 2024
095b04e
remove config context
sfc-gh-aalam Sep 30, 2024
32707f9
min-diff
sfc-gh-aalam Sep 30, 2024
3bf678d
min-diff
sfc-gh-aalam Sep 30, 2024
3eade1a
min-diff
sfc-gh-aalam Sep 30, 2024
1850d5d
Merge branch 'aalam-SNOW-1418523-make-internal-session-variables-thre…
sfc-gh-aalam Oct 2, 2024
1fa6ad2
add warnings
sfc-gh-aalam Oct 2, 2024
7c85432
Merge branch 'aalam-SNOW-1418523-make-internal-session-variables-thre…
sfc-gh-aalam Oct 3, 2024
f994842
address feedback
sfc-gh-aalam Oct 3, 2024
4621836
address feedback
sfc-gh-aalam Oct 3, 2024
e1c68f3
fix string
sfc-gh-aalam Oct 3, 2024
e5b48dd
ignore on multi-thread
sfc-gh-aalam Oct 3, 2024
496e2be
undo ignore
sfc-gh-aalam Oct 3, 2024
980d3b7
update warning message
sfc-gh-aalam Oct 4, 2024
54a6b5d
address comments
sfc-gh-aalam Oct 4, 2024
67609e8
address comments
sfc-gh-aalam Oct 4, 2024
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
179 changes: 99 additions & 80 deletions src/snowflake/snowpark/_internal/analyzer/analyzer.py

Large diffs are not rendered by default.

112 changes: 95 additions & 17 deletions src/snowflake/snowpark/_internal/analyzer/snowflake_plan.py
Original file line number Diff line number Diff line change
Expand Up @@ -312,15 +312,14 @@ def children_plan_nodes(self) -> List[Union["Selectable", "SnowflakePlan"]]:
else:
return []

def replace_repeated_subquery_with_cte(self) -> "SnowflakePlan":
def replace_repeated_subquery_with_cte(
self, cte_optimization_enabled: bool, query_compilation_stage_enabled: bool
) -> "SnowflakePlan":
# parameter protection
# the common subquery elimination will be applied if cte_optimization is not enabled
# and the new compilation stage is not enabled. When new compilation stage is enabled,
# the common subquery elimination will be done through the new plan transformation.
if (
not self.session._cte_optimization_enabled
or self.session._query_compilation_stage_enabled
):
if not cte_optimization_enabled or query_compilation_stage_enabled:
return self

# if source_plan or placeholder_query is none, it must be a leaf node,
Expand Down Expand Up @@ -355,8 +354,14 @@ def replace_repeated_subquery_with_cte(self) -> "SnowflakePlan":
# create CTE query
final_query = create_cte_query(self, duplicate_plan_set)

with self.session._lock:
# copy depends on the cte_optimization_enabled value. We should keep it
# consistent with the current context.
original_cte_optimization = self.session.cte_optimization_enabled
self.session.cte_optimization_enabled = cte_optimization_enabled
plan = copy.copy(self)
self.session.cte_optimization_enabled = original_cte_optimization
# all other parts of query are unchanged, but just replace the original query
plan = copy.copy(self)
plan.queries[-1].sql = final_query
return plan

Expand Down Expand Up @@ -518,6 +523,56 @@ def add_aliases(self, to_add: Dict) -> None:
self.expr_to_alias = {**self.expr_to_alias, **to_add}


class ConfigContext:
"""Class to manage reading of configuration values from session in the context of
plan building, analysis and resolution.

Behavior:
- Inside an active context, the configuration will be read based on snapshot taken
at the context creation stage and reset when the context is exited.
- When no active context is present, the configuration will be read from the
session object directly.
"""

def __init__(self, session) -> None:
self.session = session
self.configs = {
"_query_compilation_stage_enabled",
"cte_optimization_enabled",
"eliminate_numeric_sql_value_cast_enabled",
"large_query_breakdown_complexity_bounds",
"large_query_breakdown_enabled",
}

def __getattr__(self, name: str) -> Any:
if name in self.configs:
return getattr(self.session, name)
raise AttributeError(f"ConfigContext has no attribute {name}")

def __enter__(self) -> "ConfigContext":
Copy link
Collaborator

Choose a reason for hiding this comment

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

the enter and exit only works with WithStatement, right? what happens if someone calls it without with statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you share an example of called it without with statement. One can call context_config.__enter__() and context_config.__exit__() and they will behave as any regular method call. But the purpose of these methods it to be used inside a context to call __enter__ and __exit__ methods is a less verbose manner

self._create_snapshot()
return self

def __exit__(self, exc_type, exc_val, exc_tb) -> None:
self._reset()

def _create_snapshot(self) -> "ConfigContext":
"""Reads the configuration attributes from the session object and stores them
in the context object.
"""
for name in self.configs:
setattr(self, name, getattr(self.session, name))
return self

def _reset(self) -> None:
"""Removes the configuration attributes from the context object."""
for name in self.configs:
try:
delattr(self, name)
except AttributeError:
pass


class SnowflakePlanBuilder:
def __init__(
self,
Expand All @@ -531,6 +586,7 @@ def __init__(
# on the optimized plan. During the final query generation, no schema query is needed,
# this helps reduces un-necessary overhead for the describing call.
self._skip_schema_query = skip_schema_query
self.config_context: ConfigContext = ConfigContext(session)

@SnowflakePlan.Decorator.wrap_exception
def build(
Expand Down Expand Up @@ -566,7 +622,8 @@ def build(

placeholder_query = (
sql_generator(select_child._id)
if self.session._cte_optimization_enabled and select_child._id is not None
if self.config_context.cte_optimization_enabled
and select_child._id is not None
else None
)

Expand Down Expand Up @@ -605,7 +662,7 @@ def build_binary(

placeholder_query = (
sql_generator(select_left._id, select_right._id)
if self.session._cte_optimization_enabled
if self.config_context.cte_optimization_enabled
and select_left._id is not None
and select_right._id is not None
else None
Expand Down Expand Up @@ -637,8 +694,8 @@ def build_binary(

referenced_ctes: Set[str] = set()
if (
self.session.cte_optimization_enabled
and self.session._query_compilation_stage_enabled
self.config_context.cte_optimization_enabled
and self.config_context._query_compilation_stage_enabled
):
# When the cte optimization and the new compilation stage is enabled,
# the referred cte tables are propagated from left and right can have
Expand Down Expand Up @@ -928,7 +985,10 @@ def save_as_table(
column_definition_with_hidden_columns,
)

child = child.replace_repeated_subquery_with_cte()
child = child.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)

def get_create_table_as_select_plan(child: SnowflakePlan, replace, error):
return self.build(
Expand Down Expand Up @@ -1116,7 +1176,10 @@ def create_or_replace_view(
if not is_sql_select_statement(child.queries[0].sql.lower().strip()):
raise SnowparkClientExceptionMessages.PLAN_CREATE_VIEWS_FROM_SELECT_ONLY()

child = child.replace_repeated_subquery_with_cte()
child = child.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: create_or_replace_view_statement(name, x, is_temp, comment),
child,
Expand Down Expand Up @@ -1159,7 +1222,10 @@ def create_or_replace_dynamic_table(
# should never reach here
raise ValueError(f"Unknown create mode: {create_mode}") # pragma: no cover

child = child.replace_repeated_subquery_with_cte()
child = child.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: create_or_replace_dynamic_table_statement(
name=name,
Expand Down Expand Up @@ -1462,7 +1528,10 @@ def copy_into_location(
header: bool = False,
**copy_options: Optional[Any],
) -> SnowflakePlan:
query = query.replace_repeated_subquery_with_cte()
query = query.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: copy_into_location(
query=x,
Expand All @@ -1489,7 +1558,10 @@ def update(
source_plan: Optional[LogicalPlan],
) -> SnowflakePlan:
if source_data:
source_data = source_data.replace_repeated_subquery_with_cte()
source_data = source_data.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: update_statement(
table_name,
Expand Down Expand Up @@ -1520,7 +1592,10 @@ def delete(
source_plan: Optional[LogicalPlan],
) -> SnowflakePlan:
if source_data:
source_data = source_data.replace_repeated_subquery_with_cte()
source_data = source_data.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: delete_statement(
table_name,
Expand Down Expand Up @@ -1549,7 +1624,10 @@ def merge(
clauses: List[str],
source_plan: Optional[LogicalPlan],
) -> SnowflakePlan:
source_data = source_data.replace_repeated_subquery_with_cte()
source_data = source_data.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return self.build(
lambda x: merge_statement(table_name, x, join_expr, clauses),
source_data,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,17 +113,14 @@ def __init__(
session: Session,
query_generator: QueryGenerator,
logical_plans: List[LogicalPlan],
complexity_bounds: Tuple[int, int],
) -> None:
self.session = session
self._query_generator = query_generator
self.logical_plans = logical_plans
self._parent_map = defaultdict(set)
self.complexity_score_lower_bound = (
session.large_query_breakdown_complexity_bounds[0]
)
self.complexity_score_upper_bound = (
session.large_query_breakdown_complexity_bounds[1]
)
self.complexity_score_lower_bound = complexity_bounds[0]
self.complexity_score_upper_bound = complexity_bounds[1]

def apply(self) -> List[LogicalPlan]:
if is_active_transaction(self.session):
Expand Down
33 changes: 22 additions & 11 deletions src/snowflake/snowpark/_internal/compiler/plan_compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
get_complexity_score,
)
from snowflake.snowpark._internal.analyzer.snowflake_plan import (
ConfigContext,
PlanQueryType,
Query,
SnowflakePlan,
Expand Down Expand Up @@ -46,6 +47,7 @@ class PlanCompiler:

def __init__(self, plan: SnowflakePlan) -> None:
self._plan = plan
self.config_context = ConfigContext(self._plan.session)

def should_start_query_compilation(self) -> bool:
"""
Expand All @@ -65,14 +67,18 @@ def should_start_query_compilation(self) -> bool:
return (
not isinstance(current_session._conn, MockServerConnection)
and (self._plan.source_plan is not None)
and current_session._query_compilation_stage_enabled
and self.config_context._query_compilation_stage_enabled
and (
current_session.cte_optimization_enabled
or current_session.large_query_breakdown_enabled
self.config_context.cte_optimization_enabled
or self.config_context.large_query_breakdown_enabled
)
)

def compile(self) -> Dict[PlanQueryType, List[Query]]:
with self.config_context:
return self._compile()

def _compile(self) -> Dict[PlanQueryType, List[Query]]:
if self.should_start_query_compilation():
# preparation for compilation
# 1. make a copy of the original plan
Expand All @@ -89,7 +95,7 @@ def compile(self) -> Dict[PlanQueryType, List[Query]]:
# 3. apply each optimizations if needed
# CTE optimization
cte_start_time = time.time()
if self._plan.session.cte_optimization_enabled:
if self.config_context.cte_optimization_enabled:
repeated_subquery_eliminator = RepeatedSubqueryElimination(
logical_plans, query_generator
)
Expand All @@ -102,9 +108,12 @@ def compile(self) -> Dict[PlanQueryType, List[Query]]:
]

# Large query breakdown
if self._plan.session.large_query_breakdown_enabled:
if self.config_context.large_query_breakdown_enabled:
large_query_breakdown = LargeQueryBreakdown(
self._plan.session, query_generator, logical_plans
self._plan.session,
query_generator,
logical_plans,
self.config_context.large_query_breakdown_complexity_bounds,
)
logical_plans = large_query_breakdown.apply()

Expand All @@ -124,9 +133,9 @@ def compile(self) -> Dict[PlanQueryType, List[Query]]:
total_time = time.time() - start_time
session = self._plan.session
summary_value = {
TelemetryField.CTE_OPTIMIZATION_ENABLED.value: session.cte_optimization_enabled,
TelemetryField.LARGE_QUERY_BREAKDOWN_ENABLED.value: session.large_query_breakdown_enabled,
CompilationStageTelemetryField.COMPLEXITY_SCORE_BOUNDS.value: session.large_query_breakdown_complexity_bounds,
TelemetryField.CTE_OPTIMIZATION_ENABLED.value: self.config_context.cte_optimization_enabled,
TelemetryField.LARGE_QUERY_BREAKDOWN_ENABLED.value: self.config_context.large_query_breakdown_enabled,
CompilationStageTelemetryField.COMPLEXITY_SCORE_BOUNDS.value: self.config_context.large_query_breakdown_complexity_bounds,
CompilationStageTelemetryField.TIME_TAKEN_FOR_COMPILATION.value: total_time,
CompilationStageTelemetryField.TIME_TAKEN_FOR_DEEP_COPY_PLAN.value: deep_copy_time,
CompilationStageTelemetryField.TIME_TAKEN_FOR_CTE_OPTIMIZATION.value: cte_time,
Expand All @@ -143,8 +152,10 @@ def compile(self) -> Dict[PlanQueryType, List[Query]]:
return queries
else:
final_plan = self._plan
if self._plan.session.cte_optimization_enabled:
final_plan = final_plan.replace_repeated_subquery_with_cte()
final_plan = final_plan.replace_repeated_subquery_with_cte(
self.config_context.cte_optimization_enabled,
self.config_context._query_compilation_stage_enabled,
)
return {
PlanQueryType.QUERIES: final_plan.queries,
PlanQueryType.POST_ACTIONS: final_plan.post_actions,
Expand Down
21 changes: 16 additions & 5 deletions src/snowflake/snowpark/_internal/server_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import inspect
import os
import sys
import threading
import time
from logging import getLogger
from typing import (
Expand Down Expand Up @@ -154,6 +155,8 @@ def __init__(
options: Dict[str, Union[int, str]],
conn: Optional[SnowflakeConnection] = None,
) -> None:
self._lock = threading.RLock()
self._thread_stored = threading.local()
self._lower_case_parameters = {k.lower(): v for k, v in options.items()}
self._add_application_parameters()
self._conn = conn if conn else connect(**self._lower_case_parameters)
Expand All @@ -170,7 +173,6 @@ def __init__(

if "password" in self._lower_case_parameters:
self._lower_case_parameters["password"] = None
self._cursor = self._conn.cursor()
self._telemetry_client = TelemetryClient(self._conn)
self._query_listener: Set[QueryHistory] = set()
# The session in this case refers to a Snowflake session, not a
Expand All @@ -183,6 +185,12 @@ def __init__(
"_skip_upload_on_content_match" in signature.parameters
)

@property
def _cursor(self) -> SnowflakeCursor:
if not hasattr(self._thread_stored, "cursor"):
self._thread_stored.cursor = self._conn.cursor()
return self._thread_stored.cursor

def _add_application_parameters(self) -> None:
if PARAM_APPLICATION not in self._lower_case_parameters:
# Mirrored from snowflake-connector-python/src/snowflake/connector/connection.py#L295
Expand Down Expand Up @@ -210,10 +218,12 @@ def _add_application_parameters(self) -> None:
] = get_version()

def add_query_listener(self, listener: QueryHistory) -> None:
self._query_listener.add(listener)
with self._lock:
self._query_listener.add(listener)

def remove_query_listener(self, listener: QueryHistory) -> None:
self._query_listener.remove(listener)
with self._lock:
self._query_listener.remove(listener)

def close(self) -> None:
if self._conn:
Expand Down Expand Up @@ -360,8 +370,9 @@ def upload_stream(
raise ex

def notify_query_listeners(self, query_record: QueryRecord) -> None:
for listener in self._query_listener:
listener._add_query(query_record)
with self._lock:
for listener in self._query_listener:
listener._add_query(query_record)

def execute_and_notify_query_listener(
self, query: str, **kwargs: Any
Expand Down
Loading
Loading