Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INSERT VALUES fails on negative numbers #3610

Closed
agavra opened this issue Oct 17, 2019 · 5 comments · Fixed by #3612
Closed

INSERT VALUES fails on negative numbers #3610

agavra opened this issue Oct 17, 2019 · 5 comments · Fixed by #3612
Assignees
Labels

Comments

@agavra
Copy link
Contributor

agavra commented Oct 17, 2019

Describe the bug
INSERT INTO ... VALUES expects INTEGER_VALUE or DECIMAL_VALUE, neither of which expect a negative sign:

INTEGER_VALUE
    : DIGIT+
    ;

DECIMAL_VALUE
    : DIGIT+ '.' DIGIT*
    | '.' DIGIT+
    | DIGIT+ ('.' DIGIT*)? EXPONENT
    | '.' DIGIT+ EXPONENT
    ;

This results in a parser error (see steps to reproduce)

To Reproduce
Master as of

ksql> CREATE STREAM LONGS (L BIGINT) WITH (kakfa_topic='longs', value_format='JSON', partitions=1);
ksql> INSERT INTO LONGS (L) VALUES (-1);
line 1:36: extraneous input '-' expecting {')', 'NULL', 'TRUE', 'FALSE', STRING, INTEGER_VALUE, DECIMAL_VALUE}
Statement: INSERT INTO LONGS (L) VALUES (-1);
Caused by: line 1:36: extraneous input '-' expecting {')', 'NULL', 'TRUE',
	'FALSE', STRING, INTEGER_VALUE, DECIMAL_VALUE}

Expected behavior
Insert into succeeds.

Actual behaviour
Errpr

Additional context
Reported on Community Slack.

@agavra agavra added the bug label Oct 17, 2019
@agavra agavra self-assigned this Oct 17, 2019
@agavra
Copy link
Contributor Author

agavra commented Oct 17, 2019

This happens because our SELECT clauses use the valueExpression rule, which allows for a prefixed operator (which is a MINUS | PLUS). I think the proper fix here is probably to allow INSERT VALUES to sue any valueExpression.

@big-andy-coates
Copy link
Contributor

Is not the correct fix to update the definition for INTEGER_VALUE and DECIMAL_VALUE to allow for the minus sign?

I'm guessing a potential issue is that this would make some things ambiguous, but it might be worth at least trying....

@agavra
Copy link
Contributor Author

agavra commented Oct 18, 2019

@big-andy-coates I was thinking about that, but I think there are some good aspects of having them require positive numbers. number (which contains INTEGER_VALUE and DECIMAL_VALUE) is used to help define joinWindowSize, the window unit and interval on PRINT TOPIC - all of the things require positive numbers. I think the fix in #3612 is powerful enough to address this ticket, but also many more.

@big-andy-coates
Copy link
Contributor

We can add POSITIVE_INTEGER_VALUE and POSITIVE_DECIMAL_VALUE to handle those, right?

@agavra
Copy link
Contributor Author

agavra commented Oct 18, 2019

Perhaps - though I think that's a whole different subject :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants