Skip to content

Commit 596d4c6

Browse files
hatyohatyo
committed
Address normen662 comments.
- make sure to convert FieldKeyExpression#fieldName (which is internal) to user-facing name when constructing match candidates. - also, add tests for deeply nested structures with non-pb-compliant nested field names, and an index.
1 parent 43e6041 commit 596d4c6

File tree

6 files changed

+77
-28
lines changed

6 files changed

+77
-28
lines changed

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/expressions/FieldKeyExpression.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import com.apple.foundationdb.record.query.plan.cascades.KeyExpressionVisitor;
3434
import com.apple.foundationdb.record.query.plan.cascades.Quantifier;
3535
import com.apple.foundationdb.record.query.plan.cascades.expressions.ExplodeExpression;
36+
import com.apple.foundationdb.record.util.ProtoUtils;
3637
import com.google.common.collect.ImmutableList;
3738
import com.google.protobuf.Descriptors;
3839
import com.google.protobuf.Message;
@@ -57,6 +58,13 @@
5758
public class FieldKeyExpression extends BaseKeyExpression implements AtomKeyExpression, KeyExpressionWithoutChildren {
5859
private static final ObjectPlanHash BASE_HASH = new ObjectPlanHash("Field-Key-Expression");
5960

61+
/**
62+
* The internal field name used in the underlying protobuf message. This name may differ from the user-visible
63+
* identifier to ensure compliance with protobuf field naming conventions. When working with query planning or
64+
* user-facing operations, use {@link ProtoUtils#toUserIdentifier(String)} to convert this to the user-visible
65+
* form. However, when constructing physical operators that directly interact with stored protobuf messages,
66+
* this internal name should be used as-is.
67+
*/
6068
@Nonnull
6169
private final String fieldName;
6270
@Nonnull
@@ -216,7 +224,7 @@ public <S extends KeyExpressionVisitor.State, R> R expand(@Nonnull final KeyExpr
216224
public Quantifier.ForEach explodeField(@Nonnull Quantifier.ForEach baseQuantifier, @Nonnull List<String> fieldNamePrefix) {
217225
final List<String> fieldNames = ImmutableList.<String>builder()
218226
.addAll(fieldNamePrefix)
219-
.add(fieldName)
227+
.add(ProtoUtils.toUserIdentifier(fieldName))
220228
.build();
221229
switch (fanType) {
222230
case FanOut:

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/KeyExpressionExpansionVisitor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ public GraphExpansion visitExpression(@Nonnull final NestingKeyExpression nestin
242242
case None:
243243
List<String> newPrefix = ImmutableList.<String>builder()
244244
.addAll(fieldNamePrefix)
245-
.add(parent.getFieldName())
245+
.add(ProtoUtils.toUserIdentifier((parent.getFieldName())))
246246
.build();
247247
if (NullableArrayTypeUtils.isArrayWrapper(nestingKeyExpression)) {
248248
final RecordKeyExpressionProto.KeyExpression childProto = nestingKeyExpression.getChild().toKeyExpression();

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/ScalarTranslationVisitor.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,10 @@ public Value visitExpression(@Nonnull final NestingKeyExpression nestingKeyExpre
170170
final ScalarVisitorState state = getCurrentState();
171171
final List<String> fieldNamePrefix = state.getFieldNamePrefix();
172172
final KeyExpression child = nestingKeyExpression.getChild();
173+
final String parentFieldName = ProtoUtils.toUserIdentifier(parent.getFieldName());
173174
final List<String> newPrefix = ImmutableList.<String>builder()
174175
.addAll(fieldNamePrefix)
175-
.add(parent.getFieldName())
176+
.add(parentFieldName)
176177
.build();
177178
// TODO resolve type
178179
return pop(child.expand(push(state.withFieldNamePrefix(newPrefix))));

‎yaml-tests/src/test/resources/valid-identifiers.metrics.binpb‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,18 @@
1-
�
1+
�
2+
r
3+
all-testseEXPLAIN select "level0.field1"."level1$field.1"."level2$field.1" from "foo.table$nested" where id = 1�
4+
����]r ���D(!0��84@�COVERING(foo.table$nested.idx <,> -> [ID: KEY[2], level0__2field1: [level1__1field__21: [level2__1field__21: KEY[0]]]]) | FILTER _.ID EQUALS promote(@c12 AS LONG) | MAP (_.level0.field1.level1$field.1.level2$field.1 AS level2$field.1)�digraph G {
5+
fontname=courier;
6+
rankdir=BT;
7+
splines=polyline;
8+
1 [ label=<<table border="0" cellborder="1" cellspacing="0" cellpadding="8"><tr><td align="left">Value Computation</td></tr><tr><td align="left">MAP (q49.level0.field1.level1$field.1.level2$field.1 AS level2$field.1)</td></tr></table>> color="black" shape="plain" style="solid" fillcolor="black" fontname="courier" fontsize="8" tooltip="RELATION(LONG AS level2$field.1)" ];
9+
2 [ label=<<table border="0" cellborder="1" cellspacing="0" cellpadding="8"><tr><td align="left">Predicate Filter</td></tr><tr><td align="left">WHERE q45.ID EQUALS promote(@c12 AS LONG)</td></tr></table>> color="black" shape="plain" style="solid" fillcolor="black" fontname="courier" fontsize="8" tooltip="RELATION(LONG AS ID, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.1, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.2 AS level0.field1)" ];
10+
3 [ label=<<table border="0" cellborder="1" cellspacing="0" cellpadding="8"><tr><td align="left">Covering Index Scan</td></tr><tr><td align="left">range: &lt;-∞, ∞&gt;</td></tr></table>> color="black" shape="plain" style="solid" fillcolor="black" fontname="courier" fontsize="8" tooltip="RELATION(LONG AS ID, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.1, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.2 AS level0.field1)" ];
11+
4 [ label=<<table border="0" cellborder="1" cellspacing="0" cellpadding="8"><tr><td align="left">Index</td></tr><tr><td align="left">foo.table$nested.idx</td></tr></table>> color="black" shape="plain" style="filled" fillcolor="lightblue" fontname="courier" fontsize="8" tooltip="RELATION(LONG AS ID, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.1, LONG AS level2$field.1, LONG AS level2$field.2 AS level1$field.2 AS level0.field1)" ];
12+
3 -> 2 [ label=<&nbsp;q45> label="q45" color="gray20" style="bold" fontname="courier" fontsize="8" arrowhead="normal" arrowtail="none" dir="both" ];
13+
4 -> 3 [ color="gray20" style="solid" fontname="courier" fontsize="8" arrowhead="normal" arrowtail="none" dir="both" ];
14+
2 -> 1 [ label=<&nbsp;q49> label="q49" color="gray20" style="bold" fontname="courier" fontsize="8" arrowhead="normal" arrowtail="none" dir="both" ];
15+
}�
216
=
317
all-tests0EXPLAIN select "foo.tableA".* from "foo.tableA";�
418
�ϊ�5j ���&(10ĕ�8-@�COVERING(foo.tableA.idx <,> -> [foo__2tableA__2A1: KEY[0], foo__2tableA__2A2: KEY[1], foo__2tableA__2A3: KEY[2]]) | MAP (_.foo.tableA.A1 AS foo.tableA.A1, _.foo.tableA.A2 AS foo.tableA.A2, _.foo.tableA.A3 AS foo.tableA.A3)� digraph G {

‎yaml-tests/src/test/resources/valid-identifiers.metrics.yaml‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,17 @@
11
all-tests:
2+
- query: EXPLAIN select "level0.field1"."level1$field.1"."level2$field.1" from "foo.table$nested"
3+
where id = 1
4+
explain: 'COVERING(foo.table$nested.idx <,> -> [ID: KEY[2], level0__2field1: [level1__1field__21:
5+
[level2__1field__21: KEY[0]]]]) | FILTER _.ID EQUALS promote(@c12 AS LONG)
6+
| MAP (_.level0.field1.level1$field.1.level2$field.1 AS level2$field.1)'
7+
task_count: 474
8+
task_total_time_ms: 196
9+
transform_count: 114
10+
transform_time_ms: 143
11+
transform_yield_count: 33
12+
insert_time_ms: 10
13+
insert_new_count: 52
14+
insert_reused_count: 6
215
- query: EXPLAIN select "foo.tableA".* from "foo.tableA";
316
explain: 'COVERING(foo.tableA.idx <,> -> [foo__2tableA__2A1: KEY[0], foo__2tableA__2A2:
417
KEY[1], foo__2tableA__2A3: KEY[2]]) | MAP (_.foo.tableA.A1 AS foo.tableA.A1,

‎yaml-tests/src/test/resources/valid-identifiers.yamsql‎

Lines changed: 37 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@ options:
2121
supported_version: 4.8.13.0
2222
---
2323
schema_template:
24-
CREATE TYPE AS STRUCT "foo.struct"(S1 bigint, S2 bigint)
24+
create type as struct "nested.type$level2" ("level2$field.1" bigint, "level2$field.2" bigint)
25+
create type as struct "nested.type$level1" ("level1$field.1" "nested.type$level2", "level1$field.2" "nested.type$level2")
26+
create table "foo.table$nested" (id bigint, "level0.field1" "nested.type$level1", primary key(id))
27+
create index "foo.table$nested.idx" as select "level0.field1"."level1$field.1"."level2$field.1" from "foo.table$nested" order by "level0.field1"."level1$field.1"."level2$field.1"
28+
29+
create type as struct "foo.struct"(S1 bigint, S2 bigint)
2530
create table "foo.tableA"("foo.tableA.A1" bigint, "foo.tableA.A2" bigint, "foo.tableA.A3" bigint, primary key("foo.tableA.A1"))
2631
create index "foo.tableA.idx" as select "foo.tableA.A1", "foo.tableA.A2", "foo.tableA.A3" FROM "foo.tableA" order by "foo.tableA.A1", "foo.tableA.A2", "foo.tableA.A3"
2732
create index "foo.tableA.idx2" as select sum("foo.tableA.A1") FROM "foo.tableA" group by "foo.tableA.A2"
@@ -71,39 +76,43 @@ schema_template:
7176
---
7277
setup:
7378
steps:
74-
- query: INSERT INTO "foo.tableA"
75-
VALUES (1, 10, 1),
79+
- query: insert into "foo.table$nested"
80+
values (1, ((10, 20), (30, 40))),
81+
(2, ((100, 200), (300, 400))),
82+
(3, ((1000, 2000), (3000, 4000)))
83+
- query: insert into "foo.tableA"
84+
values (1, 10, 1),
7685
(2, 10, 2)
77-
- query: INSERT INTO "foo.tableB"
78-
VALUES (1, 20, (4, 40)),
86+
- query: insert into "foo.tableB"
87+
values (1, 20, (4, 40)),
7988
(2, 20, (5, 50)),
8089
(3, 20, (6, 60))
8190

82-
- query: INSERT INTO "foo.tableE"
83-
VALUES (1, [1, 2, 3], (4, 40)),
91+
- query: insert into "foo.tableE"
92+
values (1, [1, 2, 3], (4, 40)),
8493
(2, [2, 3, 4], (5, 50)),
8594
(3, [3, 4, 5], (6, 60))
86-
- query: INSERT INTO "foo$tableC"
87-
VALUES (1, 20, 1),
95+
- query: insert into "foo$tableC"
96+
values (1, 20, 1),
8897
(2, 20, 2),
8998
(3, 20, 3),
9099
(4, 20, 4)
91-
- query: INSERT INTO "__foo__tableD"
92-
VALUES (1, 20, 1),
100+
- query: insert into "__foo__tableD"
101+
values (1, 20, 1),
93102
(2, 20, 2),
94103
(3, 20, 3),
95104
(4, 20, 4)
96-
- query: INSERT INTO "my$adjacency$list"
97-
VALUES (1, -1),
105+
- query: insert into "my$adjacency$list"
106+
values (1, -1),
98107
(2, 1),
99108
(3, 1),
100109
(4, 1),
101110
(5, 2),
102111
(6, 2)
103112

104113
# Note: the insert below flips the order of the enum__1 and enum__2 columns
105-
- query: INSERT INTO "foo.enum.type"("enum_type.id", "enum_type.enum__2", "enum_type.enum__1")
106-
VALUES ( 1, 'B$C', 'A'),
114+
- query: insert into "foo.enum.type"("enum_type.id", "enum_type.enum__2", "enum_type.enum__1")
115+
values ( 1, 'B$C', 'A'),
107116
( 2, 'B$C', 'B$C'),
108117
( 3, 'B$C', 'C.D'),
109118
( 4, 'B$C', 'E__F'),
@@ -120,13 +129,17 @@ test_block:
120129
preset: single_repetition_ordered
121130
tests:
122131
-
123-
- query: INSERT INTO "foo.tableA" ("foo.tableA.A3", "foo.tableA.A1", "foo.tableA.A2") VALUES (3, 3, 10)
132+
- query: insert into "foo.tableA" ("foo.tableA.A3", "foo.tableA.A1", "foo.tableA.A2") values (3, 3, 10)
124133
- count: 1
125134
---
126135
test_block:
127136
name: all-tests
128137
preset: single_repetition_ordered
129138
tests:
139+
- # select from nested table
140+
- query: select "level0.field1"."level1$field.1"."level2$field.1" from "foo.table$nested" where id = 1
141+
- explain: "COVERING(foo.table$nested.idx <,> -> [ID: KEY[2], level0__2field1: [level1__1field__21: [level2__1field__21: KEY[0]]]]) | FILTER _.ID EQUALS promote(@c12 AS LONG) | MAP (_.level0.field1.level1$field.1.level2$field.1 AS level2$field.1)"
142+
- result: [{10}]
130143
-
131144
# qualified star
132145
- query: select "foo.tableA".* from "foo.tableA";
@@ -507,26 +520,26 @@ setup:
507520
setup:
508521
connect: "jdbc:embed:/FRL/IDENTIFIERS_PROTO_YAML?schema=TEST"
509522
steps:
510-
- query: INSERT INTO T1
511-
VALUES (1, 10, 1),
523+
- query: insert into T1
524+
values (1, 10, 1),
512525
(2, 10, 2),
513526
(3, 10, 3),
514527
(4, 10, 4),
515528
(5, 10, 5)
516-
- query: INSERT INTO T2
517-
VALUES (6, 10, 6),
529+
- query: insert into T2
530+
values (6, 10, 6),
518531
(7, 10, 7),
519532
(8, 10, 8),
520533
(9, 10, 9),
521534
(10, 10, 10)
522-
- query: INSERT INTO "__T3"
523-
VALUES (11, 10, 11, null),
535+
- query: insert into "__T3"
536+
values (11, 10, 11, null),
524537
(12, 10, 12, 'T3.E.A'),
525538
(13, 10, 13, 'T3.E.B'),
526539
(14, 10, 14, 'T3.E.C'),
527540
(15, 10, 15, 'T3.E.A')
528-
- query: INSERT INTO T4
529-
VALUES (16, (11, 16), 10, 16),
541+
- query: insert into T4
542+
values (16, (11, 16), 10, 16),
530543
(17, (11, 17), 10, 17),
531544
(18, (11, 18), 10, 18),
532545
(19, (11, 19), 10, 19),

0 commit comments

Comments
 (0)