clintropolis commented on code in PR #16741:
URL: https://github.com/apache/druid/pull/16741#discussion_r1681959797
##########
processing/src/main/java/org/apache/druid/query/operator/window/WindowFrame.java:
##########
@@ -21,159 +21,226 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
-import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
+
+import javax.annotation.Nullable;
-import java.util.Collections;
import java.util.List;
import java.util.Objects;
-import java.util.stream.Collectors;
-public class WindowFrame
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "unbounded", value =
WindowFrame.Unbounded.class),
Review Comment:
is this really the right abstraction? "unbounded" isn't really a frame type,
rather a frame start/end thing?
Why not just make a singleton instance for an 'unbounded' `Rows.class`?
(assuming this is meant to be `ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED
FOLLOWING` or whatever)
##########
processing/src/main/java/org/apache/druid/query/operator/window/WindowFrame.java:
##########
@@ -21,159 +21,226 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
-import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
+
+import javax.annotation.Nullable;
-import java.util.Collections;
import java.util.List;
import java.util.Objects;
-import java.util.stream.Collectors;
-public class WindowFrame
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "unbounded", value =
WindowFrame.Unbounded.class),
+ @JsonSubTypes.Type(name = "rows", value = WindowFrame.Rows.class),
+ @JsonSubTypes.Type(name = "groups", value = WindowFrame.Groups.class),
+})
+@SubclassesMustOverrideEqualsAndHashCode
+public interface WindowFrame
{
- public static WindowFrame unbounded()
+ static WindowFrame unbounded()
{
- return new WindowFrame(PeerType.ROWS, true, 0, true, 0, null);
+ return new WindowFrame.Unbounded();
}
- @SuppressWarnings("unused")
- public enum PeerType
+ static Rows rows(Integer lowerOffset, Integer upperOffset)
{
- ROWS,
- RANGE
+ return new WindowFrame.Rows(lowerOffset, upperOffset);
}
- // Will likely need to add the order by columns to also be able to deal with
RANGE peer type.
- private final PeerType peerType;
- private final boolean lowerUnbounded;
- private final int lowerOffset;
- private final boolean upperUnbounded;
- private final int upperOffset;
- private final List<ColumnWithDirection> orderBy;
-
- @JsonCreator
- public WindowFrame(
- @JsonProperty("peerType") PeerType peerType,
- @JsonProperty("lowUnbounded") boolean lowerUnbounded,
- @JsonProperty("lowOffset") int lowerOffset,
- @JsonProperty("uppUnbounded") boolean upperUnbounded,
- @JsonProperty("uppOffset") int upperOffset,
- @JsonProperty("orderBy") List<ColumnWithDirection> orderBy
- )
+ static Groups groups(
+ final Integer lowerOffset,
+ final Integer upperOffset,
+ final List<String> orderByColumns)
Review Comment:
nit: this could all fit on one line (also not sure why style bot isn't
flagging the ')' not being on a newline...)
##########
processing/src/main/java/org/apache/druid/query/operator/window/WindowFrame.java:
##########
@@ -21,159 +21,226 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
-import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
+
+import javax.annotation.Nullable;
-import java.util.Collections;
import java.util.List;
import java.util.Objects;
-import java.util.stream.Collectors;
-public class WindowFrame
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
Review Comment:
is this going to cause issues during rolling upgrade? old json used
`peerType` to pick rows/groups, but is probably uppercase from enum?
##########
processing/src/main/java/org/apache/druid/query/operator/window/WindowFrame.java:
##########
@@ -21,159 +21,226 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
-import org.apache.druid.query.operator.ColumnWithDirection;
+import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
+
+import javax.annotation.Nullable;
-import java.util.Collections;
import java.util.List;
import java.util.Objects;
-import java.util.stream.Collectors;
-public class WindowFrame
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "unbounded", value =
WindowFrame.Unbounded.class),
+ @JsonSubTypes.Type(name = "rows", value = WindowFrame.Rows.class),
+ @JsonSubTypes.Type(name = "groups", value = WindowFrame.Groups.class),
+})
+@SubclassesMustOverrideEqualsAndHashCode
+public interface WindowFrame
{
- public static WindowFrame unbounded()
+ static WindowFrame unbounded()
{
- return new WindowFrame(PeerType.ROWS, true, 0, true, 0, null);
+ return new WindowFrame.Unbounded();
}
- @SuppressWarnings("unused")
- public enum PeerType
+ static Rows rows(Integer lowerOffset, Integer upperOffset)
{
- ROWS,
- RANGE
+ return new WindowFrame.Rows(lowerOffset, upperOffset);
}
- // Will likely need to add the order by columns to also be able to deal with
RANGE peer type.
- private final PeerType peerType;
- private final boolean lowerUnbounded;
- private final int lowerOffset;
- private final boolean upperUnbounded;
- private final int upperOffset;
- private final List<ColumnWithDirection> orderBy;
-
- @JsonCreator
- public WindowFrame(
- @JsonProperty("peerType") PeerType peerType,
- @JsonProperty("lowUnbounded") boolean lowerUnbounded,
- @JsonProperty("lowOffset") int lowerOffset,
- @JsonProperty("uppUnbounded") boolean upperUnbounded,
- @JsonProperty("uppOffset") int upperOffset,
Review Comment:
is this going to cause compatibility issues during rolling upgrade because
on `OffsetFrame` these are replaced by `upperOffset`? I forget exactly if/where
these get sent over the wire (msq maybe?). Does `OffsetFrame` needs to accept
these old property names too just to be safe?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
