feat: display precision and scale when describing DECIMAL columns #6872
Conversation
|
@confluentinc It looks like @swist just signed our Contributor License Agreement. 👍 Always at your service, clabot |
There was a problem hiding this comment.
Thanks @swist! I think this is a great first pass at the PR - no major comments inline, some recommendations about testing and java-isms.
In general, any PR that changes something that is visible to the user in the CLI should have a copy-paste terminal session in the "testing done" so that we can see what it would look like from the user perspective. Something like:
ksql> describe s;
Name : S
Field | Type
-----------------
VAL | DECIMAL(5, 2)
-----------------
ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/util/EntityUtil.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/src/test/java/io/confluent/ksql/rest/entity/SchemaInfoTest.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/util/EntityUtilTest.java
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SchemaInfo.java
Show resolved
Hide resolved
swist
force-pushed
the
ksql-6843/show-decimal-precision-in-describe-output
branch
from
326960a to
ad6588b
Compare
| + " \"memberSchema\" : null" + NEWLINE | ||
| + " }" + NEWLINE | ||
| + " \"memberSchema\" : null," + NEWLINE | ||
| + " \"parameters\" : { }" + NEWLINE |
There was a problem hiding this comment.
Is there a less crappy way of validating json returns? Load from file? (would also be good if it was order invariant, but i'm guessing in Java it will be fixed wrt to order in which the AST is walked?)
There was a problem hiding this comment.
a less crappy way would also be when we eventually upgrade to java 13 to get multiline strings https://openjdk.java.net/jeps/355
(would also be good if it was order invariant, but i'm guessing in Java it will be fixed wrt to order in which the AST is walked?)
I guess what we could do here is load our string as a JSON object and then use jackson to compare them
swist
force-pushed
the
ksql-6843/show-decimal-precision-in-describe-output
branch
2 times, most recently
from
5ff9ece to
ad1a473
Compare
ksqldb-rest-model/src/test/java/io/confluent/ksql/rest/model/ImmutabilityTest.java
Outdated
Show resolved
Hide resolved
ksqldb-rest-model/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.fasterxml.jackson.datatype</groupId> | ||
| <artifactId>jackson-datatype-guava</artifactId> | ||
| <version>2.10.5</version> |
There was a problem hiding this comment.
This adds support for guava serdes in jackson
There was a problem hiding this comment.
nit: we keep all of the versions as properties in the ksqldb-parent pom so that we can reference them all in one place
though perhaps more importantly, I'm not sure I understand why we need the guava serdes?
There was a problem hiding this comment.
We need the guava series because jackson otherwise doesn't understand how to serde an ImmutableMap
There was a problem hiding this comment.
Can you get around that by declaring it as a Map (not ImmutableMap and just in the body of the method wrap it with ImmutableMap.copyOf()? In general, it's better practice to use interfaces in the fields/public accessors than the implementation class
There was a problem hiding this comment.
Unresolved this one as I feel that it may still be relevant.
There was a problem hiding this comment.
Ow shoot, only saw it after I pressed the big green button. I don't remember exactly, but I think it would work now that you've showed me how to mark it as @EffectivelyImmutable. Do you want me to do a follow up PR?
There was a problem hiding this comment.
no worries - I think a quick follow-up PR would be ideal. fewer dependencies is a good thing for build speed and package size :)
swist
force-pushed
the
ksql-6843/show-decimal-precision-in-describe-output
branch
from
ad1a473 to
f1bee05
Compare
swist
force-pushed
the
ksql-6843/show-decimal-precision-in-describe-output
branch
from
f1bee05 to
2af0181
Compare
Description
Fixes #6843
We want DESCRIBE queries to show the precision and scale for DECIMAL columns
It seems to me that maybe it would make sense to carry instances of
SqlBaseType on SchemaInfo instances somehow so that we could delegate
these formatting duties to them?
Testing done
Reviewer checklist