fix:Output to console the key and value that was inserted by a I… #3377

purplefox · 2019-09-18T12:22:11Z

Description

Fixes #3349
This PR introduces a change whereby the key and value that was inserted is now output to the console after a successful INSERT INTO statement.

Here's an example

INSERT INTO ratings (id, rating) VALUES (294, 8.1)
Inserted:
key:null
value:[ 294 | 8.1 ]

Testing done

Manual testing
Added a new CliTest

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

…NSERT INTO

big-andy-coates

Thanks @purplefox

I've raised a few nits for you to look at.

I'm also not sold on the output format.

INSERT INTO ratings (id, rating) VALUES (294, 8.1)
Inserted:
key:null
value:[ 294 | 8.1 ]

It might be better to have something more inline with what we'd get with a select e.g. something like:

INSERT INTO ratings (id, rating) VALUES (294, 8.1)
|  ROWKEY |  ID    | RATING |
+----------+-----+--------+
| null            | 294 | 8.1 .       |

Note, I'm not sure of the exact format.

We may be able to achieve this by renaming the new QueryResultEntity to just TableRows and then you could return one of these.

Plus, now that I'm seeing this, I'm wondering if the feedback is too much... would be happy to hear others thoughts.

I raised the original issue as I thought it might catch people out that its writing a null to the key. Maybe we could have some kind of SET SESSION VERBOSE ON or something to toggle such output?

Thoughts people?

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/MessageEntity.java

big-andy-coates · 2019-09-18T14:24:26Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/MessageEntity.java

+  }
+
+  @Override
+  public boolean equals(final Object o) {


incorrect hashCode and equals, as they're not taking into account the parent class's fields.

I'd suggest adding a unit test using EqualsTester and NullPointerTester to ensure your class is well behaved.

Looking at the other KSqlEntity subclasses, most (all?) seem to suffer from this problem.

So we should probably fix them all.

I actually wonder why hashCode() and equals() are overridden for these classes in the first place.

So we should probably fix them all.

If they are needed, then yes, but that's a different PR ;) . What's more important is ensuring any new code is correct.

I actually wonder why hashCode() and equals() are overridden for these classes in the first place.

Totally. Often its only for tests to ensure you can assert on output. I'm not normally a fan of having production code just for tests, but I can live with this. However, is it's in the production code base it should be correct!

So we should either fix this or remove it.

big-andy-coates · 2019-09-18T14:27:26Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/MessageEntity.java

+
+  public MessageEntity(
+      @JsonProperty("statementText")  final String statementText,
+      @JsonProperty("warnings")       final List<KsqlWarning> warnings,


Don't accept warnings if you're only passing null / empty list. Just remove the constructor param and pass an empty list to the super class.

I don't have warnings for insert into, but I left it in there as MessageEntity could be used by a different endpoint which does produce warnings.

It's easy enough for someone to put the warnings parameter back if they need it. The point is, at the moment its not needed, and I'm not even sure it a warning would get output in the CLI even if one was supplied.

Better to keep the API to want we require now. We may never need to pass a warning via this class...

big-andy-coates · 2019-09-18T14:28:12Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/MessageEntity.java

+  public MessageEntity(
+      @JsonProperty("statementText")  final String statementText,
+      @JsonProperty("warnings")       final List<KsqlWarning> warnings,
+      @JsonProperty("message")        final Optional<String> message) {


Why would the message not be provided? Isn't the whole point of this entity type to return a message?

Suggested change

@JsonProperty("message") final Optional<String> message) {

@JsonProperty("message") final String message) {

The idea is MessageEntity could be used by a different endpoint which produces an optional message.

E.g. maybe we decide to only print the insert results if the key is null.

I don't get it... this is a MessageEntity.... what's a MessageEntity without a message?

big-andy-coates · 2019-09-18T14:29:14Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/CustomExecutors.java

-      return Optional.empty();
+      final Optional<String> message =
+          executor.execute(statement, executionContext, serviceContext);
+      return Optional.of(new MessageEntity(statement.getStatementText(), null, message));


Suggested change

return Optional.of(new MessageEntity(statement.getStatementText(), null, message));

return executor.execute(statement, executionContext, serviceContext)

.map(msg -> new MessageEntity(statement.getStatementText(), msg));

Unfortunately the suggestion doesn't compile as is.
I think would need to:

return executor.execute(statement, executionContext, serviceContext) .map(msg -> new MessageEntity(statement.getStatementText(), (String)msg));

To do it on one line? Which seems a bit ugly.

Might be easier and clearer to split across two lines:

final Optional<String> res = executor.execute(statement, executionContext, serviceContext); return res.map(msg -> new MessageEntity(statement.getStatementText(), null, Optional.of(msg)));

Why would you need to cast msg to String? It is a String!

It's probably not compiling for you at the moment because MessageEntity takes an Optional<String>, (which, as per above, I don't think it should).

ksql-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java

ksql-cli/src/test/java/io/confluent/ksql/cli/CliTest.java

purplefox · 2019-09-19T12:49:53Z

I think it would make sense to have some kind of verbose on/off switch.

You probably don't want reams of output if, say, running a bunch of inserts via a script.

vpapavas · 2019-09-19T15:13:07Z

+1 on having a setting to turn on/off message reporting.

Does the functionality you propose only pertain to INSERT INTO statements or also INSERT INTO SELECT? For the latter, we don't want to see the rows inserted but rather just maybe how many were inserted or whether successful or not.

I agree with @big-andy-coates that if we decide to report something, using the same formatting as SELECT looks better.

Generally though, how often do people do INSERT INTO for single rows? Postgres doesn't report any message besides success/error.

agavra · 2019-09-19T15:34:17Z

Chiming in - if you want a CLI property to toggle it on/off you can piggy back on #3341 after its checked in.

Though IMO, I don't think the feedback is necessary. Postgres doesn't do it with such detail:

almog.gavra=# INSERT INTO FOO VALUES (1);
INSERT 0 1

(0 is OID and 1 is number of rows)

blueedgenick · 2019-09-19T16:27:23Z

Not a big fan of extra toggles you have to learn to get at (or suppress) extra messages, just cos we can't decide if we think this new message is really useful now that we've actually implemented - that's just lazy thinking :-)

I do think the "select-output-style" is better than the previous. Not because I'm particularly enamored of the visuals, but because it talks in terms of fields/columns, which is the world the user is working in when running a IBSERT VALUES, rather than in kafka-message constructs (key-value)

big-andy-coates · 2019-09-20T09:30:59Z

Ooooh decisions decisions... let's go with binning this for now. As nick said, inserting null is valid. So.. meh, if its not what they intended they'll quickly learn ... or hassle Robin ;)

fix:3349 Output to console the key and value that was inserted by a I…

ff91c09

…NSERT INTO

purplefox requested a review from a team as a code owner September 18, 2019 12:22

purplefox requested a review from big-andy-coates September 18, 2019 12:46

big-andy-coates reviewed Sep 18, 2019

View reviewed changes

big-andy-coates requested a review from a team September 18, 2019 14:44

fix:nits

92fb8e2

big-andy-coates closed this Sep 20, 2019

purplefox deleted the fix-3349-2 branch September 30, 2019 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix:Output to console the key and value that was inserted by a I… #3377

fix:Output to console the key and value that was inserted by a I… #3377

purplefox commented Sep 18, 2019 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Sep 18, 2019

purplefox Sep 18, 2019

purplefox Sep 18, 2019

big-andy-coates Sep 19, 2019

big-andy-coates Sep 18, 2019

purplefox Sep 18, 2019

big-andy-coates Sep 19, 2019

big-andy-coates Sep 18, 2019

purplefox Sep 18, 2019

big-andy-coates Sep 19, 2019

big-andy-coates Sep 18, 2019

purplefox Sep 18, 2019

purplefox Sep 18, 2019

big-andy-coates Sep 19, 2019

purplefox commented Sep 19, 2019

vpapavas commented Sep 19, 2019

agavra commented Sep 19, 2019 •

edited

Loading

blueedgenick commented Sep 19, 2019

big-andy-coates commented Sep 20, 2019

	@JsonProperty("message") final Optional<String> message) {
	@JsonProperty("message") final String message) {

	return Optional.of(new MessageEntity(statement.getStatementText(), null, message));
	return executor.execute(statement, executionContext, serviceContext)
	.map(msg -> new MessageEntity(statement.getStatementText(), msg));

fix:Output to console the key and value that was inserted by a I… #3377

fix:Output to console the key and value that was inserted by a I… #3377

Conversation

purplefox commented Sep 18, 2019 • edited Loading

Description

Testing done

Reviewer checklist

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purplefox commented Sep 19, 2019

vpapavas commented Sep 19, 2019

agavra commented Sep 19, 2019 • edited Loading

blueedgenick commented Sep 19, 2019

big-andy-coates commented Sep 20, 2019

purplefox commented Sep 18, 2019 •

edited

Loading

agavra commented Sep 19, 2019 •

edited

Loading