Repository navigation
fix(pandas-gbq): keep the dialect when configuration has a query section - #18775
abhishekacharya200 wants to merge 1 commit into
Conversation
run_query built {"query": {"useLegacySql": ...}} from the dialect and then
called job_config_dict.update(configuration). A configuration with its own
"query" section, such as the {"query": {"useQueryCache": False}} example in
the read_gbq docs, replaced that whole section, so useLegacySql was dropped
and google-cloud-bigquery ran the query as Standard SQL even with
dialect="legacy".
Merge the "query" section instead. An explicit useLegacySql in the
configuration still takes precedence.
There was a problem hiding this comment.
Code Review
This pull request updates gbq_connector.py to merge the user-provided "query" configuration with the existing query configuration instead of completely replacing it, ensuring that settings like the SQL dialect are preserved. It also adds comprehensive unit tests to verify this behavior. The reviewer identified a potential TypeError that could occur if the "query" key in the configuration is explicitly set to None, and provided a code suggestion to safely handle this scenario.
| query_config = {**job_config_dict["query"], **config.get("query", {})} | ||
| job_config_dict.update(config) | ||
| job_config_dict["query"] = query_config |
There was a problem hiding this comment.
If configuration contains a "query" key with a value of None, config.get("query", {}) returns None instead of the default {}. This causes a TypeError when attempting to unpack it with **. Please ensure that configuration values are validated or handled appropriately to maintain class design consistency and ensure fail-fast behavior.
| query_config = {**job_config_dict["query"], **config.get("query", {})} | |
| job_config_dict.update(config) | |
| job_config_dict["query"] = query_config | |
| config_query = config.get("query") | |
| if isinstance(config_query, dict): | |
| query_config = {**job_config_dict["query"], **config_query} | |
| job_config_dict.update(config) | |
| job_config_dict["query"] = query_config | |
| else: | |
| job_config_dict.update(config) |
References
- Setters should strictly validate that all values in a dictionary are of the expected type, and handle None values appropriately, to maintain class design consistency and ensure fail-fast behavior.
Fixes #18774
read_gbqtakes aconfigurationfor extra query options, and its docstring gives{'query': {'useQueryCache': False}}as the example.GbqConnector.run_querybuilds{"query": {"useLegacySql": self.dialect == "legacy"}}from the dialect and then callsjob_config_dict.update(configuration), so a configuration with its own"query"section replaces that whole section.useLegacySqlis dropped, and google-cloud-bigquery runs the query as Standard SQL even withdialect="legacy"orpandas_gbq.context.dialect = "legacy". The same happens when the query text is passed insideconfiguration, becauseread_gbqtakes the text out and leaves an empty"query"section behind.This merges the
"query"section ofconfigurationinto the default one instead of replacing it. An explicituseLegacySqlinconfigurationstill takes precedence, so only the case where it is missing changes.With the stand in server from the issue, which prints the options each query sends:
read_gbqcallmaindialect='legacy'useLegacySql=TrueuseLegacySql=Truedialect='legacy', configuration={'query': {'useQueryCache': False}}useLegacySql=FalseuseLegacySql=Truedialect='standard', configuration={'query': {'useQueryCache': False}}useLegacySql=FalseuseLegacySql=Falseread_gbq(None, dialect='legacy', configuration={'query': {'query': 'SELECT 1 AS x'}})useLegacySql=FalseuseLegacySql=TrueuseQueryCache=Falsereaches the server in both versions, so the other options fromconfigurationkeep working as before.Added
test_read_gbq_with_configuration_query_section_keeps_dialect,test_read_gbq_with_configuration_query_section_keeps_context_dialectandtest_read_gbq_with_query_in_configuration_keeps_dialect, which fail onmainand pass with this change, andtest_read_gbq_with_configuration_use_legacy_sql_overrides_dialect, which documents that an explicituseLegacySqlstill wins. The fulltests/unitsuite passes locally (253 passed, 1 skipped), and the noxfile'sruff==0.14.14import and format checks andflake8report nothing.