>From Shahrzad Shirazi <[email protected]>: Shahrzad Shirazi has submitted this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21616?usp=email )
Change subject: [ASTERIXDB-3785][COMP] Make the CLUSTER BY descriptor readable anywhere ...................................................................... [ASTERIXDB-3785][COMP] Make the CLUSTER BY descriptor readable anywhere - user model changes: no - storage format changes: no - interface changes: no Co-Authored-By: Claude <[email protected]> Ext-ref: MB-73589 Change-Id: Ie53dca75c8fa68a1bc1b7fdd71c96f477f683917 Reviewed-on: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21616 Reviewed-by: Hongyu Shi <[email protected]> Tested-by: Jenkins <[email protected]> Reviewed-by: Ian Maxon <[email protected]> Reviewed-by: Shahrzad Shirazi <[email protected]> Integration-Tests: Jenkins <[email protected]> --- M asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/ClusterByQueries.xml C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.1.ddl.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.10.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.2.update.sqlpp R asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.3.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.4.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.5.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.6.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.7.query.sqlpp C asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.8.query.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.9.query.sqlpp A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.10.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.3.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.4.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.5.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.6.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.7.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.8.adm A asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.9.adm M asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppClusterByVisitor.java 20 files changed, 218 insertions(+), 115 deletions(-) Approvals: Jenkins: Verified; Verified Hongyu Shi: Looks good to me, but someone else must approve Ian Maxon: Looks good to me, approved Shahrzad Shirazi: Looks good to me, but someone else must approve Anon. E. Moose #1000171: diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/ClusterByQueries.xml b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/ClusterByQueries.xml index 56c9bad..6399ee4 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/ClusterByQueries.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/ClusterByQueries.xml @@ -36,6 +36,13 @@ <output-dir compare="Text">kmeans-in-view</output-dir> </compilation-unit> </test-case> + <!-- The cluster descriptor reads like an ordinary record: the whole value is the record of its fields, a + field it lacks is MISSING, and both hold with the CLUSTER AS members variable also referenced. --> + <test-case FilePath="cluster-by"> + <compilation-unit name="kmeans-descriptor"> + <output-dir compare="Text">kmeans-descriptor</output-dir> + </compilation-unit> + </test-case> <!-- The default kmeansPP initialization on a fixture with enough clusters to expose a bad candidate-pool reduction; the shared 6-point k=2 dataset converges correctly from any seeding and so cannot. --> <test-case FilePath="cluster-by"> @@ -233,21 +240,6 @@ <output-dir compare="Text">kmeans-duplicate-points</output-dir> </compilation-unit> </test-case> - <!-- The cluster descriptor exposes exactly three fields, and the rewrite substitutes each away. A - reference that survives is either an unknown field or the descriptor used whole; both must name the - fields that exist rather than surface the rewrite's own variable as unresolved. --> - <test-case FilePath="cluster-by/negative"> - <compilation-unit name="unknown-descriptor-field"> - <output-dir compare="Text">unknown-descriptor-field</output-dir> - <expected-error>exposes only</expected-error> - </compilation-unit> - </test-case> - <test-case FilePath="cluster-by/negative"> - <compilation-unit name="bare-descriptor"> - <output-dir compare="Text">bare-descriptor</output-dir> - <expected-error>exposes only</expected-error> - </compilation-unit> - </test-case> <!-- Mixed widths and non-vector rows in one dataset, with membership written down. Declared Dimension decides who takes part, so the same rows cluster one way at 2 and the opposite way at 3. --> <test-case FilePath="cluster-by"> diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.1.ddl.sqlpp similarity index 62% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.1.ddl.sqlpp index 816f233..a00805e 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.1.ddl.sqlpp @@ -16,11 +16,15 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- The cluster descriptor reads like an ordinary record: as a whole value it is the record of its fields, and +-- a field it does not have is MISSING rather than an error. Two well-separated pairs, so the clustering is +-- unambiguous, and both centroids (0.5 and 10.5) are exact in binary floating point. +drop dataverse test if exists; +create dataverse test; +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc -cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +create type PointType as closed { + id : integer, + vec : [double] +}; +create dataset Points(PointType) primary key id; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.10.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.10.query.sqlpp new file mode 100644 index 0000000..d427ffa --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.10.query.sqlpp @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ +-- The distances must differ within a cluster. The shared fixture puts every point the same 0.5 from its +-- centroid, so a centroid read that returned one wrong-but-constant vector would still pass; here each +-- member sits at its own distance. Two collinear groups, each of three points whose mean is exact in binary +-- floating point: {0, 1, 5} averages 2 and {100, 101, 108} averages 103, so the distances within a cluster +-- are 2, 1, 3 and 3, 2, 5 -- distinct, and exact. +use test; + +from [ + {"id": 1, "vec": [0.0, 0.0]}, + {"id": 2, "vec": [1.0, 0.0]}, + {"id": 3, "vec": [5.0, 0.0]}, + {"id": 4, "vec": [100.0, 0.0]}, + {"id": 5, "vec": [101.0, 0.0]}, + {"id": 6, "vec": [108.0, 0.0]} +] as p +cluster by p.vec as sc +cluster as members +with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +select sc.cluster_id as cid, + sc.centroid as centroid, + (from members as m + select m.p.id as id, vector_distance(m.p.vec, sc.centroid, 'euclidean') as dist + order by m.p.id) as dists +order by cid; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.2.update.sqlpp similarity index 68% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.2.update.sqlpp index 816f233..b5cb7b5 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.2.update.sqlpp @@ -16,11 +16,11 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc -cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +insert into Points ([ + {"id": 1, "vec": [0.0, 0.0]}, + {"id": 2, "vec": [1.0, 0.0]}, + {"id": 3, "vec": [10.0, 10.0]}, + {"id": 4, "vec": [11.0, 10.0]} +]); diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.3.query.sqlpp similarity index 70% rename from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp rename to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.3.query.sqlpp index 816f233..3844ca2 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.3.query.sqlpp @@ -16,11 +16,12 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- The descriptor as a whole value: the record of the fields it exposes. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc +from Points as p +cluster by p.vec as sc cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +with {"clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } +select value sc +order by sc.cluster_id; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.4.query.sqlpp similarity index 70% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.4.query.sqlpp index 816f233..c847ddb 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.4.query.sqlpp @@ -16,11 +16,12 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- A field the descriptor does not have is MISSING, so the projection omits it -- as for any other record. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc +from Points as p +cluster by p.vec as sc cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +select sc.cluster_id as cid, sc.a as a +order by cid; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.5.query.sqlpp similarity index 70% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.5.query.sqlpp index 816f233..b76fd3b 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.5.query.sqlpp @@ -16,11 +16,13 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- The whole-value form alongside the CLUSTER AS members variable, which is the case the field-by-field +-- substitution was originally written to avoid. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc +from Points as p +cluster by p.vec as sc cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +select sc, array_count(members) as cnt +order by sc.cluster_id; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.6.query.sqlpp similarity index 70% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.6.query.sqlpp index 816f233..767af6e 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.6.query.sqlpp @@ -16,11 +16,15 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc +use test; + +from Points as p +cluster by p.vec as sc cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +select sc.cluster_id as cid, + (from members as m + select m.p.id as id, vector_distance(m.p.vec, sc.centroid, 'euclidean') as dist + order by m.p.id) as dists +order by cid; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.7.query.sqlpp similarity index 63% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.7.query.sqlpp index 816f233..0146abd 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.7.query.sqlpp @@ -16,11 +16,15 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- The same nesting in both forms the descriptor supports: the field on its own, and the whole value, which +-- carries the field inside the record it stands for. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc +from Points as p +cluster by p.vec as sc cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +select sc.cluster_id as cid, + (from members as m select value sc.centroid)[0] as centroid_nested, + array_count((from members as m select value sc)) as whole_reads +order by cid; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.8.query.sqlpp similarity index 66% copy from asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp copy to asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.8.query.sqlpp index 816f233..fbf2c12 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/negative/bare-descriptor/test.1.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.8.query.sqlpp @@ -16,11 +16,15 @@ * specific language governing permissions and limitations * under the License. */ --- The descriptor used as a whole value. It is not a record that survives to runtime, so it cannot be --- selected -- same leftover reference, same reason to name it clearly. +-- SELECT * over the block sees the block's own bindings and nothing the rewrite invented: the cluster id and +-- the CLUSTER AS members variable, two fields, not the binding the centroid is computed into. +use test; -from [{"v": [1.0, 2.0]}, {"v": [3.0, 4.0]}] as p -cluster by p.v as sc -cluster as members -with { "clustering_algorithm": "K-Means", "dimension": [2], "num_clusters": 2 } -select value sc; +select value array_count(object_names(c)) +from ( + from Points as p + cluster by p.vec as sc + cluster as members + with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } + select * +) as c; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.9.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.9.query.sqlpp new file mode 100644 index 0000000..8cb6405 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/cluster-by/kmeans-descriptor/kmeans-descriptor.9.query.sqlpp @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ +-- End to end in the shape the case came in as: a WITH feeding a CLUSTER BY block whose SELECT measures each +-- member against its own cluster's centroid, and SELECT * over the result. The filter and the names are +-- trimmed to the fixture; what is being exercised is the nesting -- a descriptor read inside a subquery over +-- the CLUSTER AS members, two query levels down from where the clustering happens. +use test; + +with subset as ( + select p.id as id, p.vec as vec + from Points as p + where p.id > 0 +), +clustered as ( + select sc.cluster_id as cid, + array_count(members) as cnt, + (from members as m + select m.i.id as id, vector_distance(m.i.vec, sc.centroid, 'euclidean') as dist + order by m.i.id) as dists + from subset as i + cluster by i.vec as sc + cluster as members + with {"clustering_algorithm": "K-Means","dimension": [2], "num_clusters": 2 } +) +select * from clustered +order by clustered.cid; diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.10.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.10.adm new file mode 100644 index 0000000..d91f0e0 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.10.adm @@ -0,0 +1,2 @@ +{ "cid": 0, "centroid": [ 2.0, 0.0 ], "dists": [ { "id": 1, "dist": 2.0 }, { "id": 2, "dist": 1.0 }, { "id": 3, "dist": 3.0 } ] } +{ "cid": 1, "centroid": [ 103.0, 0.0 ], "dists": [ { "id": 4, "dist": 3.0 }, { "id": 5, "dist": 2.0 }, { "id": 6, "dist": 5.0 } ] } \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.3.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.3.adm new file mode 100644 index 0000000..1240709 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.3.adm @@ -0,0 +1,2 @@ +{ "cluster_id": 0, "centroid": [ 0.5, 0.0 ] } +{ "cluster_id": 1, "centroid": [ 10.5, 10.0 ] } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.4.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.4.adm new file mode 100644 index 0000000..478a504 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.4.adm @@ -0,0 +1,2 @@ +{ "cid": 0 } +{ "cid": 1 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.5.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.5.adm new file mode 100644 index 0000000..018d91e --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.5.adm @@ -0,0 +1,2 @@ +{ "sc": { "cluster_id": 0, "centroid": [ 0.5, 0.0 ] }, "cnt": 2 } +{ "sc": { "cluster_id": 1, "centroid": [ 10.5, 10.0 ] }, "cnt": 2 } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.6.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.6.adm new file mode 100644 index 0000000..8232ddd --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.6.adm @@ -0,0 +1,2 @@ +{ "cid": 0, "dists": [ { "id": 1, "dist": 0.5 }, { "id": 2, "dist": 0.5 } ] } +{ "cid": 1, "dists": [ { "id": 3, "dist": 0.5 }, { "id": 4, "dist": 0.5 } ] } diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.7.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.7.adm new file mode 100644 index 0000000..a21046b --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.7.adm @@ -0,0 +1,2 @@ +{ "cid": 0, "whole_reads": 2, "centroid_nested": [ 0.5, 0.0 ] } +{ "cid": 1, "whole_reads": 2, "centroid_nested": [ 10.5, 10.0 ] } \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.8.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.8.adm new file mode 100644 index 0000000..a5c8806 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.8.adm @@ -0,0 +1,2 @@ +3 +3 diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.9.adm b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.9.adm new file mode 100644 index 0000000..08383b0 --- /dev/null +++ b/asterixdb/asterix-app/src/test/resources/runtimets/results/cluster-by/kmeans-descriptor/kmeans-descriptor.9.adm @@ -0,0 +1,2 @@ +{ "clustered": { "cid": 0, "cnt": 2, "dists": [ { "id": 1, "dist": 0.5 }, { "id": 2, "dist": 0.5 } ] } } +{ "clustered": { "cid": 1, "cnt": 2, "dists": [ { "id": 3, "dist": 0.5 }, { "id": 4, "dist": 0.5 } ] } } \ No newline at end of file diff --git a/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppClusterByVisitor.java b/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppClusterByVisitor.java index 6dbb64c..e5e8704 100644 --- a/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppClusterByVisitor.java +++ b/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/SqlppClusterByVisitor.java @@ -22,6 +22,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -42,12 +43,15 @@ import org.apache.asterix.lang.common.clause.WhereClause; import org.apache.asterix.lang.common.expression.CallExpr; import org.apache.asterix.lang.common.expression.FieldAccessor; +import org.apache.asterix.lang.common.expression.FieldBinding; import org.apache.asterix.lang.common.expression.GbyVariableExpressionPair; import org.apache.asterix.lang.common.expression.IndexAccessor; import org.apache.asterix.lang.common.expression.LiteralExpr; import org.apache.asterix.lang.common.expression.OperatorExpr; +import org.apache.asterix.lang.common.expression.RecordConstructor; import org.apache.asterix.lang.common.expression.VariableExpr; import org.apache.asterix.lang.common.literal.IntegerLiteral; +import org.apache.asterix.lang.common.literal.StringLiteral; import org.apache.asterix.lang.common.rewrites.LangRewritingContext; import org.apache.asterix.lang.common.struct.Identifier; import org.apache.asterix.lang.common.struct.OperatorType; @@ -308,7 +312,17 @@ selectBlock.setGroupbyClause(mainGby); if (cbc.hasClusterDescriptorVar()) { - substituteDescriptorFields(selectExpression, cbc, clusteringExpr, labelExpr, loc); + // The centroid is a SQL-92 aggregate, so substituting it in place puts one wherever the descriptor is + // read -- inside a subquery over the CLUSTER AS members that earns the subquery an implicit GROUP BY + // (SqlppGroupByVisitor#rewriteSelectWithoutGroupBy), which replaces its scope and leaves the + // subquery's own FROM binding undefined. Bound once here instead, so every read -- field access and + // whole-value record alike -- is a variable reference, safe at any depth. Post-group: an aggregate + // needs the grouping, and a PRE-group LET would reach the CLUSTER AS members record. + VarIdentifier centroidVar = context.newVariable(); + selectBlock.getLetHavingListAfterGroupby().add( + letClause(centroidVar, call(BuiltinFunctions.SCALAR_CENTROID, loc, copy(clusteringExpr)), loc)); + + substituteDescriptorFields(selectExpression, cbc, centroidVar, labelExpr, loc); } } @@ -453,59 +467,36 @@ } /** - * Replaces every {@code <descriptor>.<field>} read with the expression that computes it. The descriptor is - * substituted field by field rather than bound to a record: an OpenRecordConstructor here breaks type - * inference when the members variable is also referenced. For the same reason {@code sc.centroid} becomes - * {@code centroid(vec)} as a group aggregate rather than an index into the centroid list, keeping every - * post-group descriptor field on the group-aggregation path. + * Replaces every descriptor read with the expression that computes it: each {@code <descriptor>.<field>} + * by its own, and the descriptor read as a whole value by a record of them all. {@code sc.centroid} + * becomes {@code centroid(vec)} as a group aggregate, keeping it on the group-aggregation path. + * <p> + * One map covers both, so the descriptor reads like any other record: substitution matches + * outermost-first, so a known field access is replaced whole and an unknown one falls through to the bare + * variable inside it, becoming a field of the record -- MISSING, as {@code r.nosuchfield} is anywhere. + * The record is inlined at each use rather than bound to a LET, so it is the shape the group-by pipeline + * already handles (as TPCH Q1 writes {@code {"sum_qty": sum(quantity), ...}} under its own GROUP AS). */ private void substituteDescriptorFields(SelectExpression selectExpression, ClusterbyClause cbc, - Expression clusteringExpr, Expression labelExpr, SourceLocation loc) throws CompilationException { + VarIdentifier centroidVar, Expression labelExpr, SourceLocation loc) throws CompilationException { VariableExpr scVar = cbc.getClusterDescriptorVar(); - Map<Expression, Expression> scSubst = new HashMap<>(); - scSubst.put(fieldAccess(scVar, SC_CLUSTER_ID, loc), copy(labelExpr)); - scSubst.put(fieldAccess(scVar, SC_CENTROID, loc), - call(BuiltinFunctions.SCALAR_CENTROID, loc, copy(clusteringExpr))); - SqlppRewriteUtil.substituteExpression(selectExpression, scSubst, context); - // Substitution replaced every field the descriptor actually has. Anything still referring to it is - // either an unknown field or the descriptor used as a whole value, neither of which survives to - // runtime -- and left alone both reach the user as a bare "unresolved identifier" naming a variable - // the rewrite invented. Say what it is instead. - checkDescriptorResolved(selectExpression, scVar, loc); - } + // Named once, so a new field reaches both the field accesses and the whole-value record. + Map<String, Expression> fields = new LinkedHashMap<>(); + fields.put(SC_CLUSTER_ID, copy(labelExpr)); + fields.put(SC_CENTROID, varRef(centroidVar, loc)); - /** - * Raises when the query still refers to the cluster descriptor after the rewrite substituted its fields - * away -- {@code sc.somethingElse}, or {@code sc} on its own. Without this the leftover variable reaches - * the resolver as an undefined identifier, which names the rewrite's own variable rather than telling the - * user which field they asked for. - */ - private static void checkDescriptorResolved(ILangExpression expr, VariableExpr descriptorVar, SourceLocation loc) - throws CompilationException { - DescriptorLeftoverFinder finder = new DescriptorLeftoverFinder(descriptorVar); - expr.accept(finder, null); - if (finder.found) { - throw new CompilationException(ErrorCode.COMPILATION_ERROR, loc, - "CLUSTER BY cluster descriptor '" + descriptorVar.getVar().getValue() + "' exposes only " - + SC_FIELDS_DISPLAY + ", and cannot be referenced as a whole value."); + List<FieldBinding> fbList = new ArrayList<>(fields.size()); + Map<Expression, Expression> subst = new HashMap<>(); + for (Map.Entry<String, Expression> field : fields.entrySet()) { + subst.put(fieldAccess(scVar, field.getKey(), loc), field.getValue()); + LiteralExpr nameLit = new LiteralExpr(new StringLiteral(field.getKey())); + nameLit.setSourceLocation(loc); + fbList.add(new FieldBinding(nameLit, copy(field.getValue()))); } - } - - private static final class DescriptorLeftoverFinder extends AbstractSqlppSimpleExpressionVisitor { - private final VariableExpr descriptorVar; - private boolean found; - - private DescriptorLeftoverFinder(VariableExpr descriptorVar) { - this.descriptorVar = descriptorVar; - } - - @Override - public Expression visit(VariableExpr v, ILangExpression arg) throws CompilationException { - if (descriptorVar.getVar().getValue().equals(v.getVar().getValue())) { - found = true; - } - return super.visit(v, arg); - } + RecordConstructor descriptorRecord = new RecordConstructor(fbList); + descriptorRecord.setSourceLocation(loc); + subst.put(new VariableExpr(scVar.getVar()), descriptorRecord); + SqlppRewriteUtil.substituteExpression(selectExpression, subst, context); } private FieldAccessor fieldAccess(VariableExpr recordVar, String field, SourceLocation loc) { -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21616?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ie53dca75c8fa68a1bc1b7fdd71c96f477f683917 Gerrit-Change-Number: 21616 Gerrit-PatchSet: 16 Gerrit-Owner: Shahrzad Shirazi <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Hongyu Shi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Shahrzad Shirazi <[email protected]>
