Fixes chdb-ds: UrlTableFunction.to_sql() emits HEADERS outside url() call#563
Merged
Merged
Conversation
|
|
Contributor
|
Hi @nklmish, thanks a lot for spotting this issue and providing the fix. Your changes are fully reasonable. I added escape handling, class comments and new test cases. Also fixed the edge case where format_name is empty but structure is set. Previously, the URL table function would ignore the structure parameter in this case. |
Contributor
Author
|
Thank you @wudidapaopao for your kind support with adding new test cases and documentation ! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Per the ClickHouse
url()table function reference, headers must be the 4th positional argument inside the function call, with structure as the 3rd positional argument (defaulting to'auto'when not provided — chdb rejects''withCode: 36 Table structure is empty).Changes
sql_paramsbeforef"url(...)"construction so they end up insideurl()as the 4th positional arg'auto'viasql_params.extend(["'auto'"] * (3 - len(sql_params)))"Key: Value"strings to'Key'='Value'pairs per ClickHouse'sheaders()function syntaxdictinput directly (headers={"Authorization": "Bearer xyz"})Tests
New file
datastore/tests/test_url_table_function_headers.pywithclass TestUrlTableFunctionHeaders(8 methods, matching the existingtest_table_functions.pyclass-based convention):test_to_sql_no_headers_unchanged— regression guard for no-headers casetest_to_sql_with_headers_inside_url_call— confirms headers emit insideurl()test_to_sql_headers_pad_structure_auto— structure auto-padded when format providedtest_to_sql_headers_pad_format_and_structure_auto— both auto-padded when neither providedtest_to_sql_headers_dict_input— dict input supporttest_to_sql_headers_multiple_entries— multiple headerstest_to_sql_headers_missing_url_raises— confirms the new headers code path doesn't suppress the existing url-required validationtest_to_sql_headers_executes_via_chdb— end-to-end runtime check: the generated SQL parses and executes viachdb.query()againsthttps://httpbin.org/ip(skips gracefully if chdb or network unavailable)Updated existing
test_url_with_headers_listandtest_url_with_headers_stringintest_table_functions.py— they were asserting the broken output, now assert the correct fixed shape.Verification
datastore/tests/tests PASS (no regressions; 9 skipped, 85 xfailed, 4 xpassed all unchanged from baseline)https://httpbin.org/ipwith chdb 4.1.6 (Python 3.13, macOS arm64) — returns expected JSON, no parse errorIssue
#564