Skip to content

Commit 3e2a5d7

Browse files
nvamelichevnvamelichev
committed
noticket: Minor YqlStatementPart cleanup
- Stricter default implementation of `YqlStatementPart.combine()` that throws `UnsupportedOperationException` - Removed the (incorrect and strange) `YqlOrderBy.combine()` implementation that ignores `this.sortKeys` *and* does not attempt to de-duplicate sort keys, allowing multiple occurrences of the same sort key, even with a different sort direction. Known YOJ users are unlikely to have used the `YqlOrderBy.combine()` functionality, ever. - Better function signature for `YqlStatement.mergeParts()` that combines `YqlStatementPart`s of the same type. It no longer takes a `Stream` of `YqlStatementPart`s but a `Collection` of them, which is appropriate for most usages. Implementations of custom statements (especially inheriting from `PredicateStatement`) will need to eventually transition to the new `mergeParts` variant. - Improved consistency in `YqlLimit` and `YqlView`: use `getXyz()` POJO-style getters instead of `xyz()`, deprecate the confusingly named `YqlLimit.size()` method in favor of explicit `getLimit()` and `getOffset()` getters which are more general.
1 parent e6c11f7 commit 3e2a5d7

File tree

9 files changed

+97
-64
lines changed

9 files changed

+97
-64
lines changed

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/statement/CountAllStatement.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ public String getQuery(String tablespace) {
2828
return declarations()
2929
+ "SELECT COUNT(*) AS count"
3030
+ " FROM " + table(tablespace)
31-
+ " " + mergeParts(parts.stream())
31+
+ " " + mergeParts(parts)
3232
.sorted(comparing(YqlStatementPart::getPriority))
3333
.map(sp -> sp.toFullYql(schema))
3434
.map(this::resolveParamNames)

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/statement/FindStatement.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public String getQuery(String tablespace) {
5454
return declarations()
5555
+ "SELECT " + (distinct ? "DISTINCT " : "") + outNames()
5656
+ " FROM " + table(tablespace)
57-
+ " " + mergeParts(parts.stream())
57+
+ " " + mergeParts(parts)
5858
.sorted(comparing(YqlStatementPart::getPriority))
5959
.map(sp -> sp.toFullYql(schema))
6060
.map(this::resolveParamNames)

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/statement/YqlStatement.java

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import com.google.protobuf.NullValue;
55
import lombok.Getter;
66
import tech.ydb.proto.ValueProtos;
7+
import tech.ydb.yoj.DeprecationWarnings;
78
import tech.ydb.yoj.databind.schema.Schema;
89
import tech.ydb.yoj.repository.db.Entity;
910
import tech.ydb.yoj.repository.db.EntityIdSchema;
@@ -78,13 +79,44 @@ public String getDeclaration(String name, String type) {
7879
return String.format("DECLARE %s AS %s;\n", name, type);
7980
}
8081

82+
/**
83+
* Tries to combine/simplify the specified {@link YqlStatementPart statement part}s into a potentially smaller number of statement parts, e.g.,
84+
* joining multiple {@code YqlPredicate}s into a single {@code AND} clause ({@code AndPredicate}).
85+
* <br>Note that this method does not attempt to sort statement parts by {@link YqlStatementPart#getPriority() priority} or perform any
86+
* YQL code generation at all.
87+
* <p><strong>Warning:</strong> A closed/consumed or a partially consumed {@code Stream} could have <em>potentially</em> been passed
88+
* to this method. But in all cases that we know of (both standard and custom {@code YqlStatement}s), this method was always fed a fresh stream,
89+
* obtained by calling {@code someCollection.stream()}. This method is now replaced by the less error-prone {@link #mergeParts(Collection)}.
90+
*
91+
* @deprecated This method is deprecated and will be removed in YOJ 3.0.0. Please use {@link #mergeParts(Collection)} instead.
92+
*/
93+
@Deprecated(forRemoval = true)
94+
@SuppressWarnings("DataFlowIssue")
8195
protected static Stream<? extends YqlStatementPart<?>> mergeParts(Stream<? extends YqlStatementPart<?>> origParts) {
96+
DeprecationWarnings.warnOnce("YqlStatement.mergeParts(Stream)",
97+
"YqlStatement.mergeParts(Stream) is deprecated and will be removed in YOJ 3.0.0. Use YqlStatement.mergeParts(Collection) instead");
8298
return origParts
8399
.collect(groupingBy(YqlStatementPart::getType))
84100
.values().stream()
85101
.flatMap(items -> combine(items).stream());
86102
}
87103

104+
/**
105+
* Tries to combine/simplify the specified {@link YqlStatementPart statement part}s into a potentially smaller number of statement parts, e.g.,
106+
* joining multiple {@code YqlPredicate}s into a single {@code AND} clause ({@code AndPredicate}).
107+
* <br>Note that this method does not attempt to sort statement parts by {@link YqlStatementPart#getPriority() priority} or perform any
108+
* YQL code generation at all.
109+
*
110+
* @param origParts original collection of {@link YqlStatementPart statement parts}
111+
* @return a fresh stream containing potentially combined {@link YqlStatementPart statement parts}
112+
*/
113+
protected static Stream<? extends YqlStatementPart<?>> mergeParts(Collection<? extends YqlStatementPart<?>> origParts) {
114+
return origParts.stream()
115+
.collect(groupingBy(YqlStatementPart::getType))
116+
.values().stream()
117+
.flatMap(items -> combine(items).stream());
118+
}
119+
88120
private static List<? extends YqlStatementPart<?>> combine(List<? extends YqlStatementPart<?>> items) {
89121
if (items.size() < 2) {
90122
return items;

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlLimit.java

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,30 @@
11
package tech.ydb.yoj.repository.ydb.yql;
22

33
import com.google.common.base.Preconditions;
4-
import lombok.EqualsAndHashCode;
54
import lombok.NonNull;
5+
import lombok.Value;
66
import lombok.With;
7-
import lombok.experimental.FieldDefaults;
7+
import tech.ydb.yoj.DeprecationWarnings;
88
import tech.ydb.yoj.repository.db.Entity;
99
import tech.ydb.yoj.repository.db.EntitySchema;
1010

11-
import java.util.List;
12-
13-
import static lombok.AccessLevel.PRIVATE;
14-
1511
/**
1612
* Represents a {@code LIMIT ... [OFFSET ...]} clause in a YQL statement.
13+
* <br>Note that YDB does <strong>not</strong> support {@code OFFSET} without {@code LIMIT}, so "row offset" cannot be set directly.
14+
* To return the maximum possible amount of rows, you must know the YDB ResultSet row limit (e.g., defined by {@code YDB_KQP_RESULT_ROWS_LIMIT}
15+
* environment variable for local YDB-in-Docker), and use {@link YqlLimit#range(long, long) YqlLimit.range(offset, max rows + offset)}.
16+
* If you have a specific limit {@code < max rows}, it's much better to use {@link YqlLimit#range(long, long) YqlLimit.range(offset, limit + offset)},
17+
* of course.
1718
*
1819
* @see #top(long)
1920
* @see #range(long, long)
2021
* @see #toYql(EntitySchema)
2122
*/
22-
@EqualsAndHashCode
23-
@FieldDefaults(makeFinal = true, level = PRIVATE)
24-
public final class YqlLimit implements YqlStatementPart<YqlLimit> {
23+
@Value
24+
public class YqlLimit implements YqlStatementPart<YqlLimit> {
25+
/**
26+
* Gives a {@code LIMIT} clause that will always return an empty range ({@code LIMIT 0}).
27+
*/
2528
public static final YqlLimit EMPTY = new YqlLimit(0, 0);
2629

2730
@With
@@ -40,17 +43,15 @@ private YqlLimit(long limit, long offset) {
4043
* Creates a limit clause to fetch rows in the half-open range {@code [from, to)}.
4144
*
4245
* @param from first row index, counting from 0, inclusive
43-
* @param to last row index, counting from 0, exclusive
44-
* @return limit clause to fetch rows in range {@code [from, to)}
46+
* @param to last row index, counting from 0, exclusive. Must be {@code >= from}
47+
* @return limit clause to fetch {@code (to - from)} rows in range {@code [from, to)}
4548
*/
4649
public static YqlLimit range(long from, long to) {
4750
Preconditions.checkArgument(from >= 0, "from must be >= 0");
4851
Preconditions.checkArgument(to >= 0, "to must be >= 0");
4952
Preconditions.checkArgument(to >= from, "to must be >= from");
5053

51-
long limit = to - from;
52-
long offset = limit == 0 ? 0 : from;
53-
return limit == 0 ? EMPTY : new YqlLimit(limit, offset);
54+
return to == from ? EMPTY : new YqlLimit(to - from, from);
5455
}
5556

5657
/**
@@ -73,11 +74,22 @@ public static YqlLimit empty() {
7374
return EMPTY;
7475
}
7576

77+
/**
78+
* @deprecated Please calculate the maximum number of rows fetched by this {@code LIMIT} clause by using {@code YqlLimit.getLimit()} and
79+
* {@code YqlLimit.getOffset()} instead.
80+
*/
81+
@Deprecated(forRemoval = true)
7682
public long size() {
83+
DeprecationWarnings.warnOnce("YqlLimit.size()",
84+
"Please calculate range size using YqlLimit.getLimit() and YqlLimit.getOffset() instead of calling YqlLimit.size()");
7785
return limit;
7886
}
7987

88+
/**
89+
* @return {@code true} if this {@code YqlLimit} represents an empty range ({@code LIMIT 0}); {@code false} otherwise
90+
*/
8091
public boolean isEmpty() {
92+
// Does not need to check the offset because with limit == 0, the offset is irrelevant, the DB will return no rows anyway!
8193
return limit == 0;
8294
}
8395

@@ -98,12 +110,7 @@ public String getYqlPrefix() {
98110

99111
@Override
100112
public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
101-
return size() + (offset == 0 ? "" : " OFFSET " + offset);
102-
}
103-
104-
@Override
105-
public List<? extends YqlStatementPart<?>> combine(@NonNull List<? extends YqlLimit> other) {
106-
throw new UnsupportedOperationException("Multiple LIMIT specifications are not supported");
113+
return limit + (offset == 0 ? "" : " OFFSET " + offset);
107114
}
108115

109116
@Override

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlOrderBy.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -124,13 +124,6 @@ public int getPriority() {
124124
return 100;
125125
}
126126

127-
@Override
128-
public List<? extends YqlOrderBy> combine(@NonNull List<? extends YqlOrderBy> other) {
129-
return singletonList(new YqlOrderBy(other.stream()
130-
.flatMap(o -> o.getKeys().stream())
131-
.collect(toList())));
132-
}
133-
134127
@Override
135128
public String toString() {
136129
return format("order by %s", keys.stream().map(Object::toString).collect(joining(", ")));

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlPredicate.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ public final List<? extends YqlStatementPart<?>> combine(@NonNull List<? extends
229229
}
230230

231231
/**
232-
* @return stream of statement parameter specifications, if this YQL predicate uses parameters
232+
* @return a fresh stream of statement parameter specifications, if this YQL predicate uses parameters; an fresh empty stream otherwise
233233
*/
234234
public Stream<YqlPredicateParam<?>> paramStream() {
235235
return Stream.empty();

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlStatementPart.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,6 @@ default <T extends Entity<T>> String toFullYql(@NonNull EntitySchema<T> schema)
2020
<T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema);
2121

2222
default List<? extends YqlStatementPart<?>> combine(@NonNull List<? extends P> other) {
23-
return other;
23+
throw new UnsupportedOperationException("Multiple " + getType() + " specifications are not supported");
2424
}
2525
}

‎repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/yql/YqlView.java

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,39 @@
11
package tech.ydb.yoj.repository.ydb.yql;
22

3-
import com.google.common.base.Preconditions;
43
import com.google.common.base.Strings;
5-
import lombok.EqualsAndHashCode;
64
import lombok.NonNull;
5+
import lombok.RequiredArgsConstructor;
6+
import lombok.Value;
77
import lombok.With;
8-
import lombok.experimental.FieldDefaults;
8+
import tech.ydb.yoj.DeprecationWarnings;
99
import tech.ydb.yoj.databind.schema.Schema;
1010
import tech.ydb.yoj.repository.db.Entity;
1111
import tech.ydb.yoj.repository.db.EntitySchema;
1212

13-
import java.util.List;
14-
1513
import static lombok.AccessLevel.PRIVATE;
1614

1715
/**
1816
* Represents a {@code view [index_name]} clause in a YQL statement, i.e. index usage
1917
*
2018
* @see #toYql(EntitySchema)
2119
*/
22-
@EqualsAndHashCode
23-
@FieldDefaults(makeFinal = true, level = PRIVATE)
20+
@Value
21+
@RequiredArgsConstructor(access = PRIVATE)
2422
public class YqlView implements YqlStatementPart<YqlView> {
2523
public static final String TYPE = "VIEW";
2624
public static final YqlView EMPTY = new YqlView("");
2725

2826
@With
27+
@NonNull
2928
String index;
3029

31-
private YqlView(String index) {
32-
Preconditions.checkArgument(index != null, "index cannot be null");
33-
this.index = index;
34-
}
35-
3630
/**
3731
* Creates a view clause to fetch rows using effective index
3832
*
39-
* @param index index
33+
* @param index index name; must not be {@code null}
4034
* @return view clause to fetch rows using index
4135
*/
42-
public static YqlView index(String index) {
36+
public static YqlView index(@NonNull String index) {
4337
return new YqlView(index);
4438
}
4539

@@ -51,7 +45,14 @@ public static YqlView empty() {
5145
return EMPTY;
5246
}
5347

48+
/**
49+
* @deprecated This method is confusingly named, because there also is a static constructor {@link YqlView#index(String)}.
50+
* It will be removed in YOJ 3.0.0. Please use {@link #getIndex()} instead.
51+
*/
52+
@Deprecated(forRemoval = true)
5453
public String index() {
54+
DeprecationWarnings.warnOnce("YqlView.index()",
55+
"YqlView.index() getter will be removed in YOJ 3.0.0. Please use YqlView.getIndex() instead");
5556
return index;
5657
}
5758

@@ -86,11 +87,6 @@ public <T extends Entity<T>> String toYql(@NonNull EntitySchema<T> schema) {
8687
);
8788
}
8889

89-
@Override
90-
public List<? extends YqlStatementPart<?>> combine(@NonNull List<? extends YqlView> other) {
91-
throw new UnsupportedOperationException("Multiple VIEW specifications are not supported");
92-
}
93-
9490
@Override
9591
public String toString() {
9692
return "view [" + index + "]";

‎repository-ydb-v2/src/test/java/tech/ydb/yoj/repository/ydb/YqlLimitTest.java

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
package tech.ydb.yoj.repository.ydb;
22

3-
import lombok.Value;
43
import org.junit.Test;
5-
import tech.ydb.yoj.repository.db.Entity;
64
import tech.ydb.yoj.repository.db.EntitySchema;
5+
import tech.ydb.yoj.repository.db.RecordEntity;
76
import tech.ydb.yoj.repository.ydb.yql.YqlLimit;
87

98
import static java.util.Collections.singletonList;
@@ -16,7 +15,7 @@ public class YqlLimitTest {
1615
public void nonempty_top() {
1716
YqlLimit lim = YqlLimit.top(100L);
1817

19-
assertThat(lim.size()).isEqualTo(100L);
18+
assertThat(lim.getLimit()).isEqualTo(100L);
2019
assertThat(lim.isEmpty()).isFalse();
2120
assertThat(lim.toYql(emptySchema)).isEqualToIgnoringCase("100");
2221
}
@@ -30,7 +29,17 @@ public void empty_top() {
3029
public void empty_range_without_offset() {
3130
YqlLimit lim = YqlLimit.range(0, 0);
3231

33-
assertThat(lim.size()).isEqualTo(0L);
32+
assertThat(lim).isEqualTo(YqlLimit.EMPTY);
33+
assertThat(lim.getLimit()).isEqualTo(0L);
34+
assertThat(lim.isEmpty()).isTrue();
35+
assertThat(lim.toYql(emptySchema)).isEqualTo("0");
36+
}
37+
38+
@Test
39+
public void empty_range_without_offset_2() {
40+
YqlLimit lim = YqlLimit.empty();
41+
42+
assertThat(lim.getLimit()).isEqualTo(0L);
3443
assertThat(lim.isEmpty()).isTrue();
3544
assertThat(lim.toYql(emptySchema)).isEqualTo("0");
3645
}
@@ -39,7 +48,7 @@ public void empty_range_without_offset() {
3948
public void nonempty_range_without_offset() {
4049
YqlLimit lim = YqlLimit.range(0, 27);
4150

42-
assertThat(lim.size()).isEqualTo(27L);
51+
assertThat(lim.getLimit()).isEqualTo(27L);
4352
assertThat(lim.isEmpty()).isFalse();
4453
assertThat(lim.toYql(emptySchema)).isEqualTo("27");
4554
}
@@ -48,7 +57,8 @@ public void nonempty_range_without_offset() {
4857
public void empty_range_with_offset() {
4958
YqlLimit lim = YqlLimit.range(5, 5);
5059

51-
assertThat(lim.size()).isEqualTo(0L);
60+
assertThat(lim).isEqualTo(YqlLimit.EMPTY);
61+
assertThat(lim.getLimit()).isEqualTo(0L);
5262
assertThat(lim.isEmpty()).isTrue();
5363
assertThat(lim.toYql(emptySchema)).isEqualTo("0");
5464
}
@@ -57,7 +67,7 @@ public void empty_range_with_offset() {
5767
public void nonempty_range_with_offset() {
5868
YqlLimit lim = YqlLimit.range(5, 7);
5969

60-
assertThat(lim.size()).isEqualTo(2L);
70+
assertThat(lim.getLimit()).isEqualTo(2L);
6171
assertThat(lim.isEmpty()).isFalse();
6272
assertThat(lim.toYql(emptySchema)).isEqualTo("2 OFFSET 5");
6373
}
@@ -78,13 +88,8 @@ public void multiple_limit_specs_are_not_supported() {
7888
lim.combine(singletonList(YqlLimit.range(0, 200)));
7989
}
8090

81-
@Value
82-
private static final class EmptyEntity implements Entity<EmptyEntity> {
83-
private Id id;
84-
85-
@Value
86-
private static final class Id implements Entity.Id<EmptyEntity> {
87-
int value2;
91+
private record EmptyEntity(Id id) implements RecordEntity<EmptyEntity> {
92+
private record Id(int value2) implements RecordEntity.Id<EmptyEntity> {
8893
}
8994
}
9095
}

0 commit comments

Comments
 (0)