[v1] Remove PartiQLValue from AST; refactor AST literals #1650
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| package org.partiql.ast.literal; | ||
|
|
||
| import org.jetbrains.annotations.NotNull; | ||
| import org.partiql.ast.AstNode; | ||
| import org.partiql.ast.AstVisitor; | ||
| import org.partiql.ast.DataType; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.math.BigInteger; | ||
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
||
| public abstract class Literal extends AstNode { | ||
|
There was a problem hiding this comment. Latest design currently models things a bit differently than before. Key summary is
There was a problem hiding this comment.
That's a good point, and I didn't think of that. We wouldn't be able to zero-pad and would need to insert nulls for those fields. Seems like a lot of extra work (on read aka scribe) when a (rule,string) is sufficient for capturing syntax which ultimately is the goal. There was a problem hiding this comment.
Yeah that was my mindset w/ this latest refactor -- AST at the end of the day just captures syntax with some slight validation w/ the parser. I think most users are not actually performing computation directly on the AST. Computation (like in our case) will come later in the plan or even pushed all the way to the evaluator. There was a problem hiding this comment. Because of the flexibility PType provides with the fat interface, I explicitly wrote in the Javadocs that consumers of this should not author their own implementations. See Could add that to this API as well. There was a problem hiding this comment. I ended up changing this to not be an abstract class and a concrete class extending |
||
| @NotNull | ||
| public abstract LiteralKind kind(); | ||
|
There was a problem hiding this comment. Not exactly a fat-interface but similar pattern! We are doing a tagged-union, and we don't actually want a LiteralKind enum and the code (aka tag) should go directly on the Literal (like we did with DataType). There was a problem hiding this comment. I'll try using the code directly on the Literal. I had ran into some slightly repetitive + confusing code pattern when using a LiteralKind: ^ There was a problem hiding this comment. Changed to be more like |
||
|
|
||
| @Override | ||
| @NotNull | ||
| public Collection<AstNode> children() { | ||
| return Collections.emptyList(); | ||
| } | ||
|
There was a problem hiding this comment. In a quick follow-up PR, could you please change this to This way it has ordered semantics and the There was a problem hiding this comment. Sure. I'll track this as a followup in #1610. |
||
|
|
||
| @Override | ||
| public <R, C> R accept(@NotNull AstVisitor<R, C> visitor, C ctx) { | ||
| return visitor.visitLiteral(this, ctx); | ||
| } | ||
|
|
||
| // Factory methods | ||
| public static Literal litApprox(@NotNull String value) { | ||
| return new LiteralApprox(value); | ||
| } | ||
|
|
||
| public static Literal litBool(boolean value) { | ||
| return new LiteralBool(value); | ||
| } | ||
|
|
||
| public static Literal litExact(@NotNull BigDecimal value) { | ||
| if (value.scale() == 0) { | ||
| return new LiteralExact(value + "."); | ||
| } else { | ||
| return new LiteralExact(value.toString()); | ||
| } | ||
| } | ||
|
|
||
| public static Literal litExact(@NotNull String value) { | ||
| return new LiteralExact(value); | ||
| } | ||
|
|
||
| public static Literal litInt(int value) { | ||
| return new LiteralInt(Integer.toString(value)); | ||
| } | ||
|
|
||
| public static Literal litInt(long value) { | ||
| return new LiteralInt(Long.toString(value)); | ||
| } | ||
|
|
||
| public static Literal litInt(@NotNull BigInteger value) { | ||
| return new LiteralInt(value.toString()); | ||
| } | ||
|
|
||
| public static Literal litInt(@NotNull String value) { | ||
| return new LiteralInt(value); | ||
| } | ||
|
|
||
| public static Literal litNull() { | ||
| return new LiteralNull(); | ||
| } | ||
|
|
||
| public static Literal litMissing() { | ||
| return new LiteralMissing(); | ||
| } | ||
|
|
||
| public static Literal litString(@NotNull String value) { | ||
| return new LiteralString(value); | ||
| } | ||
|
|
||
| public static Literal litTypedString(@NotNull DataType type, @NotNull String value) { | ||
| return new LiteralTypedString(type, value); | ||
| } | ||
alancai98 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. If you want to be as restrictive as possible, you can have explicit There was a problem hiding this comment. I still think there's some value in having a public factory method to construct typed string literals w/ arguments We could always add such methods in the future. |
||
|
|
||
| // Value extraction | ||
| /** | ||
| * TODO docs | ||
| * Valid for just LiteralBool | ||
| */ | ||
| public boolean booleanValue() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * TODO docs | ||
| * Valid for just LiteralApprox, LiteralInt, and LiteralExact | ||
| */ | ||
| @NotNull | ||
| public String numberValue() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * TODO docs | ||
| * Valid for just LiteralInt and LiteralExact | ||
| */ | ||
| @NotNull | ||
| public BigDecimal bigDecimalValue() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * TODO docs | ||
| * Valid for just LiteralString and LiteralTypedString | ||
| */ | ||
| @NotNull | ||
| public String stringValue() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
| * TODO docs | ||
| * Valid for just LiteralTypedString | ||
| */ | ||
| @NotNull | ||
| public DataType dataType() { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package org.partiql.ast.literal; | ||
|
There was a problem hiding this comment. I discussed a different modeling with @RCHowell using just one class rather than three for the numerics ( public class LiteralNumber extends Literal {
@Nullable
private final Long p1;
@Nullable
private final BigDecimal p2;
@NotNull
public final ParseContext kind;
private LiteralNumber(@Nullable Long p1, @Nullable BigDecimal p2, @NotNull ParseContext kind) {
this.p1 = p1;
this.p2 = p2;
this.kind = kind;
}
// Factory methods
public static LiteralNumber integer(long value) {
return new LiteralNumber(value, null, ParseContext.INTEGER);
}
public static LiteralNumber integer(int value) {
return new LiteralNumber((long) value, null, ParseContext.INTEGER);
}
public static LiteralNumber exact(BigDecimal value) {
return new LiteralNumber(null, value, ParseContext.EXACT);
}
public static LiteralNumber approx(BigDecimal value, long exponent) {
return new LiteralNumber(exponent, value, ParseContext.APPROX);
}
public static LiteralNumber approx(BigDecimal value) {
return new LiteralNumber(null, value, ParseContext.APPROX);
}
public static LiteralNumber approx(float value) {
return approx(BigDecimal.valueOf(value));
}
public static LiteralNumber approx(double value) {
return approx(BigDecimal.valueOf(value));
}
// Getting the value out
@NotNull
public BigDecimal getDecimal() {
if (kind == ParseContext.INTEGER) {
return p2;
} else {
throw new IllegalStateException("Unknown context: " + kind);
}
}
public long getInteger() {
if (kind == ParseContext.INTEGER) {
return p1;
} else {
throw new IllegalStateException("Unknown context: " + kind);
}
}
// similarly for getDouble
// TODO if we keep this representation, change to extend `AstEnum`
public enum ParseContext {
INTEGER,
EXACT,
APPROX
}
@NotNull
@Override
public String getText() {
switch (kind) {
// Since they're nullable, can be slightly annoying to extract the value but it's internal code
case INTEGER:
assert p1 != null;
return p1.toString();
case EXACT:
assert p2 != null;
return p2.toString();
case APPROX:
assert p1 != null;
assert p2 != null;
return p2 + "e" + p1;
default:
throw new IllegalStateException("Unknown context: " + kind);
}
}
}and if we wanted to internalize the enum, could add some additional methods public boolean isInteger() {
return kind == ParseContext.INTEGER;
}
public boolean isExact() {
return kind == ParseContext.EXACT;
}
public boolean isApprox() {
return kind == ParseContext.APPROX;
}There was a problem hiding this comment. imo, the customer code using these different modelings is pretty similar. But I feel like the internal implementation code is simpler if we break apart the Using the single class with an enum val v = when (lit) {
is LiteralNumber -> {
val kind = lit.kind
when (kind) {
LiteralNumber.ParseContext.EXACT -> {
lit.decimal
}
LiteralNumber.ParseContext.INTEGER -> {
lit.integer
}
LiteralNumber.ParseContext.APPROX -> {
lit.double
}
else -> error("Unexpected numeric literal: $lit")
}
}
else -> error("Unexpected literal: $lit")
}Not using the enum directly but using helper methods val v2 = when (lit) {
is LiteralNumber -> {
if (lit.isExact) {
lit.decimal
}
else if (lit.isInteger) {
lit.integer
}
else if (lit.isApprox) {
lit.double
} else {
error("Unexpected numeric literal: $lit")
}
}
else -> error("Unexpected literal: $lit")
}Using current PR's three classes val v3 = when (lit) {
is LiteralInteger -> lit.integer
is LiteralExact -> lit.decimal
is LiteralApprox -> lit.double
else -> error("Unexpected literal: $lit")
}There was a problem hiding this comment. Sent a comment elsewhere, but we don't have to do the null checks. This is internal code which will throw a NPE if we have a bug whereas now it throws an assertion exception, again if we have a bug. The checks don't gain us anything because an exception is thrown either way. |
||
|
|
||
| import lombok.EqualsAndHashCode; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| @EqualsAndHashCode(callSuper = false) | ||
| class LiteralApprox extends Literal { | ||
| @NotNull | ||
| String value; | ||
|
|
||
| LiteralApprox(@NotNull String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public String numberValue() { | ||
| return value; | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public LiteralKind kind() { | ||
| return LiteralKind.NUM_APPROX(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package org.partiql.ast.literal; | ||
|
|
||
| import lombok.EqualsAndHashCode; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| @EqualsAndHashCode(callSuper = false) | ||
| class LiteralBool extends Literal { | ||
| private final boolean value; | ||
|
|
||
| LiteralBool(boolean value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean booleanValue() { | ||
| return value; | ||
| } | ||
|
|
||
| @NotNull | ||
| @Override | ||
| public LiteralKind kind() { | ||
| return LiteralKind.BOOLEAN(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(self-review) partiql-ast now will not depend on other packages. Had to add the spi dependency to partiql-parser following this change