Skip to content

Commit

Permalink
Remove redundent brackets for the join syntax (#649)
Browse files Browse the repository at this point in the history 
* remove redundant brackets

* add relationship test for clickhouse

* enable dynamic-query by default

* fix checkstyle

* fix dry plan test

* remove redundant limit
  • Loading branch information
@goldmedal
goldmedal authored Jul 4, 2024
1 parent a7de461 commit 9af4fbc
Show file tree
Hide file tree
Showing 7 changed files with 183 additions and 20 deletions.
126 changes: 122 additions & 4 deletions ibis-server/tests/routers/ibis/test_clickhouse.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,24 @@ def clickhouse(request) -> ClickHouseContainer:
os.path.dirname(__file__), "../../resource/tpch/data/orders.parquet"
)
client.insert_df("orders", pd.read_parquet(data_path))
client.command("""
CREATE TABLE customer (
c_custkey Int32,
c_name String,
c_address String,
c_nationkey Int32,
c_phone String,
c_acctbal Decimal(15,2),
c_mktsegment String,
c_comment String
)
ENGINE = MergeTree
ORDER BY (c_custkey)
""")
data_path = os.path.join(
os.path.dirname(__file__), "../../resource/tpch/data/customer.parquet"
)
client.insert_df("customer", pd.read_parquet(data_path))
request.addfinalizer(ch.stop)
return ch

Expand Down Expand Up @@ -87,9 +105,48 @@ class TestClickHouse:
"expression": "toDateTime64('2024-01-01T23:59:59', 9, 'UTC')",
"type": "timestamp",
},
{
"name": "customer",
"type": "Customer",
"relationship": "OrdersCustomer",
},
{
"name": "customer_name",
"type": "varchar",
"isCalculated": True,
"expression": "customer.name",
},
],
"primaryKey": "orderkey",
}
},
{
"name": "Customer",
"refSql": "select * from test.customer",
"columns": [
{"name": "custkey", "expression": "c_custkey", "type": "integer"},
{"name": "name", "expression": "c_name", "type": "varchar"},
{
"name": "orders",
"type": "Orders",
"relationship": "OrdersCustomer",
},
{
"name": "totalprice",
"type": "float",
"isCalculated": True,
"expression": "sum(orders.totalprice)",
},
],
"primaryKey": "custkey",
},
],
"relationships": [
{
"name": "OrdersCustomer",
"models": ["Orders", "Customer"],
"joinType": "MANY_TO_ONE",
"condition": "Orders.custkey = Customer.custkey",
},
],
}

Expand Down Expand Up @@ -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
assert len(result["data"]) == 1
assert result["data"][0] == [
1,
Expand All @@ -133,6 +190,7 @@ def test_query(self, clickhouse: ClickHouseContainer):
"1_370",
1704153599000,
1704153599000,
"Customer#000000370",
]
assert result["dtypes"] == {
"orderkey": "int32",
Expand All @@ -143,6 +201,7 @@ def test_query(self, clickhouse: ClickHouseContainer):
"order_cust_key": "object",
"timestamp": "datetime64[ns]",
"timestamptz": "datetime64[ns, UTC]",
"customer_name": "object",
}

def test_query_with_connection_url(self, clickhouse: ClickHouseContainer):
Expand All @@ -157,7 +216,7 @@ def test_query_with_connection_url(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
assert len(result["data"]) == 1
assert result["data"][0][0] == 1
assert result["dtypes"] is not None
Expand All @@ -180,7 +239,7 @@ def test_query_with_column_dtypes(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
assert len(result["data"]) == 1
assert result["data"][0] == [
1,
Expand All @@ -191,6 +250,7 @@ def test_query_with_column_dtypes(self, clickhouse: ClickHouseContainer):
"1_370",
"2024-01-01 23:59:59.000000",
"2024-01-01 23:59:59.000000 UTC",
"Customer#000000370",
]
assert result["dtypes"] == {
"orderkey": "int32",
Expand All @@ -201,6 +261,7 @@ def test_query_with_column_dtypes(self, clickhouse: ClickHouseContainer):
"order_cust_key": "object",
"timestamp": "object",
"timestamptz": "object",
"customer_name": "object",
}

def test_query_with_limit(self, clickhouse: ClickHouseContainer):
Expand Down Expand Up @@ -231,6 +292,63 @@ def test_query_with_limit(self, clickhouse: ClickHouseContainer):
result = response.json()
assert len(result["data"]) == 1

def test_query_join(self, clickhouse: ClickHouseContainer):
connection_info = self.to_connection_info(clickhouse)
response = client.post(
url=f"{self.base_url}/query",
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',
},
)
assert response.status_code == 200
result = response.json()
assert len(result["columns"]) == 1
assert len(result["data"]) == 1
assert result["data"][0] == ["Customer#000000370"]
assert result["dtypes"] == {
"customer_name": "object",
}

def test_query_to_one_relationship(self, clickhouse: ClickHouseContainer):
connection_info = self.to_connection_info(clickhouse)
response = client.post(
url=f"{self.base_url}/query",
json={
"connectionInfo": connection_info,
"manifestStr": self.manifest_str,
"sql": 'SELECT customer_name FROM "Orders" where custkey = 370 LIMIT 1',
},
)
assert response.status_code == 200
result = response.json()
assert len(result["columns"]) == 1
assert len(result["data"]) == 1
assert result["data"][0] == ["Customer#000000370"]
assert result["dtypes"] == {
"customer_name": "object",
}

def test_query_to_many_relationship(self, clickhouse: ClickHouseContainer):
connection_info = self.to_connection_info(clickhouse)
response = client.post(
url=f"{self.base_url}/query",
json={
"connectionInfo": connection_info,
"manifestStr": self.manifest_str,
"sql": 'SELECT totalprice FROM "Customer" where custkey = 370 LIMIT 1',
},
)
assert response.status_code == 200
result = response.json()
assert len(result["columns"]) == 1
assert len(result["data"]) == 1
assert result["data"][0] == [2860895.79]
assert result["dtypes"] == {
"totalprice": "object",
}

def test_query_without_manifest(self, clickhouse: ClickHouseContainer):
connection_info = self.to_connection_info(clickhouse)
response = client.post(
Expand Down
7 changes: 0 additions & 7 deletions trino-parser/src/main/java/io/trino/sql/SqlFormatter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,9 +562,6 @@ protected Void visitJoin(Join node, Integer indent)
type = "NATURAL " + type;
}

if (node.getType() != Join.Type.IMPLICIT) {
builder.append('(');
}
process(node.getLeft(), indent);

builder.append('\n');
Expand Down Expand Up @@ -594,10 +591,6 @@ else if (!(criteria instanceof NaturalJoin)) {
}
}

if (node.getType() != Join.Type.IMPLICIT) {
builder.append(")");
}

return null;
}

Expand Down
49 changes: 49 additions & 0 deletions trino-parser/src/test/java/io/trino/sql/TestSqlFormatter.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.trino.sql;

import io.trino.sql.parser.ParsingOptions;
import io.trino.sql.parser.SqlParser;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class TestSqlFormatter
{
private static final SqlParser SQL_PARSER = new SqlParser();

@Test
public void testFormatJoin()
{
String sql = "SELECT * FROM a JOIN b ON a.x = b.y";
String formattedSql = SqlFormatter.formatSql(SQL_PARSER.createStatement(sql, new ParsingOptions()));
assertEquals("""
SELECT *
FROM
a
INNER JOIN b ON (a.x = b.y)
""", formattedSql);

sql = "SELECT * FROM a JOIN b ON a.x = b.y JOIN c ON a.x = c.z";
formattedSql = SqlFormatter.formatSql(SQL_PARSER.createStatement(sql, new ParsingOptions()));
assertEquals("""
SELECT *
FROM
a
INNER JOIN b ON (a.x = b.y)
INNER JOIN c ON (a.x = c.z)
""", formattedSql);
}
}
2 changes: 1 addition & 1 deletion wren-base/src/main/java/io/wren/base/config/WrenConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public enum DataSourceType

private File wrenMDLDirectory = new File("etc/mdl");
private DataSourceType dataSourceType = DataSourceType.DUCKDB;
private boolean enableDynamicFields;
private boolean enableDynamicFields = true;

@NotNull
public File getWrenMDLDirectory()
Expand Down
3 changes: 3 additions & 0 deletions wren-main/src/main/java/io/wren/main/PreviewService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import com.google.common.collect.Streams;
import com.google.inject.Inject;
import io.airlift.log.Logger;
import io.wren.base.AnalyzedMDL;
import io.wren.base.Column;
import io.wren.base.ConnectorRecordIterator;
Expand All @@ -50,6 +51,7 @@

public class PreviewService
{
private static final Logger LOG = Logger.get(PreviewService.class);
private final Metadata metadata;

private final SqlConverter sqlConverter;
Expand Down Expand Up @@ -101,6 +103,7 @@ public CompletableFuture<String> dryPlan(WrenMDL mdl, String sql, boolean isMode

String planned = WrenPlanner.rewrite(sql, sessionContext, new AnalyzedMDL(mdl, null));
if (isModelingOnly) {
LOG.info("Planned SQL: %s", planned);
return planned;
}
return sqlConverter.convert(planned, sessionContext);
Expand Down
8 changes: 4 additions & 4 deletions wren-tests/src/test/java/io/wren/testing/TestMDLResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ public void testDryRunAndDryPlan()
, "Orders"."custkey" "custkey"
, "Orders_relationsub"."customer_name" "customer_name"
FROM
((
(
SELECT
"Orders"."orderkey" "orderkey"
, "Orders"."custkey" "custkey"
Expand All @@ -245,7 +245,7 @@ LEFT JOIN (
"Orders"."orderkey"
, "Customer"."name" "customer_name"
FROM
((
(
SELECT
o_orderkey "orderkey"
, o_custkey "custkey"
Expand All @@ -256,8 +256,8 @@ LEFT JOIN (
tpch.orders
) "Orders"
) "Orders"
LEFT JOIN "Customer" ON ("Customer"."custkey" = "Orders"."custkey"))
) "Orders_relationsub" ON ("Orders"."orderkey" = "Orders_relationsub"."orderkey"))
LEFT JOIN "Customer" ON ("Customer"."custkey" = "Orders"."custkey")
) "Orders_relationsub" ON ("Orders"."orderkey" = "Orders_relationsub"."orderkey")
)\s
SELECT customer_name
FROM
Expand Down
8 changes: 4 additions & 4 deletions wren-tests/src/test/java/io/wren/testing/TestMDLResourceV2.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void testDryPlan()
, "Orders"."custkey" "custkey"
, "Orders_relationsub"."customer_name" "customer_name"
FROM
((
(
SELECT
"Orders"."orderkey" "orderkey"
, "Orders"."custkey" "custkey"
Expand All @@ -171,7 +171,7 @@ LEFT JOIN (
"Orders"."orderkey"
, "Customer"."name" "customer_name"
FROM
((
(
SELECT
o_orderkey "orderkey"
, o_custkey "custkey"
Expand All @@ -182,8 +182,8 @@ LEFT JOIN (
tpch.orders
) "Orders"
) "Orders"
LEFT JOIN "Customer" ON ("Customer"."custkey" = "Orders"."custkey"))
) "Orders_relationsub" ON ("Orders"."orderkey" = "Orders_relationsub"."orderkey"))
LEFT JOIN "Customer" ON ("Customer"."custkey" = "Orders"."custkey")
) "Orders_relationsub" ON ("Orders"."orderkey" = "Orders_relationsub"."orderkey")
)\s
SELECT customer_name
FROM
Expand Down

0 comments on commit 9af4fbc

Please sign in to comment.