Skip to content

Remove redundent brackets for the join syntax #649

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
merged 6 commits into from
Jul 4, 2024

Conversation

goldmedal
Copy link
Contributor

@goldmedal goldmedal commented Jul 3, 2024

Description

Close #647
The join relation with brackets is valid for Trino but isn't for Clickhouse.

SELECT *
FROM (
  a
  INNER JOIN b ON (a.x = b.y)
)

Because ibis can't handle the syntax for Clickhouse, we have stopped generating the join syntax with brackets in this PR.

SELECT *
FROM
  a
  INNER JOIN b ON (a.x = b.y)

Related Changed

  • Currenlty, dynamic-query will be enable by default for wren-engine.

@@ -122,7 +179,7 @@ def test_query(self, clickhouse: ClickHouseContainer):
)
assert response.status_code == 200
result = response.json()
assert len(result["columns"]) == len(self.manifest["models"][0]["columns"])
assert len(result["columns"]) == 9
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a constant instead of an object reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the model has calculated fields now, the number of columns will be different. In dynamic query mode, the calculated fields won't be selected when select all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Thanks.

Comment on lines 299 to 303
params={"limit": 1},
json={
"connectionInfo": connection_info,
"manifestStr": self.manifest_str,
"sql": 'SELECT name as customer_name FROM "Orders" join "Customer" on "Orders".custkey = "Customer".custkey WHERE custkey = 370 LIMIT 1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Two limit could keep only one.

@grieve54706 grieve54706 merged commit 9af4fbc into main Jul 4, 2024
4 checks passed
@grieve54706 grieve54706 deleted the feature/remove-rudent-brackets branch July 4, 2024 02:55
grieve54706 pushed a commit that referenced this pull request Dec 13, 2024
* remove redundant brackets

* add relationship test for clickhouse

* enable dynamic-query by default

* fix checkstyle

* fix dry plan test

* remove redundant limit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClickHouse connector: can not use () after from closure
2 participants