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-1878372: Fix analyzer access across threads #2912

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sfc-gh-aalam
Copy link
Contributor

@sfc-gh-aalam sfc-gh-aalam commented Jan 22, 2025

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-1878372

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
  3. Please describe how your code solves the related issue.

    In this change, we make the following changes to Selectable:

    • the analyzer object is accessed via a thread so that each selectable uses the analyzer created for the dedicated thread.
    • the analyzer can be set to a different analyzer only during optimization stage when _is_valid_for_replacement=True.

We also update Analyzer to initialize alias_maps_to_use with an empty dict {} instead of None. In single threaded-case, resolve would initialize alias_maps_to_use to {} and then use it for analyze but in multithreaded-case, it is possible for one thread to call resolve and a different thread to call analyze.

@github-actions github-actions bot added the local testing Local Testing issues/PRs label Jan 22, 2025
@sfc-gh-aalam sfc-gh-aalam marked this pull request as ready for review January 23, 2025 18:05
@sfc-gh-aalam sfc-gh-aalam requested review from a team as code owners January 23, 2025 18:05
@@ -121,7 +121,7 @@ def test_selectable_entity_individual_node_complexity(mock_session, mock_analyze
assert plan_node.individual_node_complexity == {PlanNodeCategory.COLUMN: 1}


def test_select_sql_individual_node_complexity(mock_analyzer):
def test_select_sql_individual_node_complexity(mock_session, mock_analyzer):
Copy link
Contributor

Choose a reason for hiding this comment

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

What does adding mock_session do for these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good question, calling mock session here sets mock_analyzer.session=mock_session.

@pytest.fixture(scope="module")
def mock_session(mock_analyzer) -> Session:
fake_session = mock.create_autospec(Session)
fake_session._cte_optimization_enabled = False
fake_session._analyzer = mock_analyzer
fake_session._plan_lock = mock.MagicMock()
mock_analyzer.session = fake_session
return fake_session

this is probably not the best way to do it. open to suggestions.

@@ -84,7 +84,7 @@ def test_find_duplicate_subtrees(test_case):
assert repeated_node_complexity == expected_repeated_node_complexity


def test_encode_node_id_with_query_select_sql(mock_analyzer):
def test_encode_node_id_with_query_select_sql(mock_session, mock_analyzer):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as jamison's question. Linking old response: #2912 (comment)

Comment on lines -458 to +459
assert new_replaced_plan.analyzer == mock_query_generator
# new_replaced_plan is created with QueryGenerator.to_selectable
assert new_replaced_plan.analyzer == mock_analyzer
Copy link
Contributor

@sfc-gh-aling sfc-gh-aling Jan 23, 2025

Choose a reason for hiding this comment

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

is this change to pass the new "setter" check?
is mock_query_generator/mock_analyzer used in any of the test code after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new_replaced_plan is created by calling within QueryGenerator.to_selectable which uses self as an analyzer.

Before this change Selectable.analyzer would have returned the analyzer used to create the selectable i.e. mock_query_generator.

After this change, Selectable.analyzer will return the appropriate session.analyzer. Therefore, we need to update this test.

@sfc-gh-aalam sfc-gh-aalam requested a review from a team January 24, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
local testing Local Testing issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants