fix: create persistent queries fail with reserved words

[spena]

Description

Fixes #3202

This PR adds quotes around the column identifier that uses reserved words when the identifier is brought from another stream using a CSAS/CSAT.

Testing done

• Add a unit test in the SqlFormatterTest to validate the new CSAS columns come with quotes.
• Verify it manually

ksql> CREATE STREAM TEST (START STRING, `END` STRING) WITH (KAFKA_TOPIC='test_topic', value_format='JSON');

ksql> CREATE STREAM S1 AS SELECT `START`, `END` FROM TEST;

 Message                                                               
-----------------------------------------------------------------------
 Stream created and running. Created by query with query ID: CSAS_S1_5 
-----------------------------------------------------------------------

ksql> select * from s1;
+---------------------------------------------+---------------------------------------------+---------------------------------------------+---------------------------------------------+
|ROWTIME                                      |ROWKEY                                       |START                                        |END                                          |
+---------------------------------------------+---------------------------------------------+---------------------------------------------+---------------------------------------------+
|1565799422152                                |null                                         |2019-04-08T15:50.10                          |2019-04-08T15:50.31                          |

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 #")

[apurvam]

Thanks for the patch @spena . Since we have a passing test case, for this exact scenario, I wonder if this is also a regression due to the recent changes to execute the modified query string (through the SqlFormatter). I recall we also dropped the WINDOW clauses for the same reason.

@agavra what do you think?

[spena]

@apurvam The test case you linked is not valid. I found out that QTT tests do not validate requests sent to the REST endpoints, which is where this issue happened. This issue was introduced by some injectors called by the RequestValidator which is called by KsqlResource. QTT calls the EngineExecutor directly instead, so the reason it never caught this regression.

[agavra]

@apurvam - it's the same category of bug. #3180 refactored the QTTs so that it never went through the code path that tested this (rendering the work done in #3037 void) and I believe #3194 may have caused this specific issue.

EDIT: I might be wrong here, looks like this issue was present in 5.3.0 which is before any of the above.

@spena - as part of this PR, can you make sure the QTTs go back through SqlFormatInjector? That change (#3037) was meant to catch exactly these bugs.

[apurvam]

@agavra, so is it the same category of bug or not? your first statement suggests so, your edit makes that ambiguous.

[agavra]

I'll look into it and figure it out. I'll also add a negative test case that is expected to fail in QTT to make sure a future refactor won't mess this up again.

[agavra]

Okay we were hit with a double whammy here:

1. The refactor that I mentioned made it so that the SqlFormatInjector wasn't being hit
2. Even if SqlFormatInjector was being hit, if there was an exception during formatting it just ignored it and returned the original statement. I have no idea what possessed me to implement it that way, but that explains why it didn't catch this the first time around.

I will create a PR for these changes.

[apurvam]

Thanks @agavra . So then this the same class of regression as the one where we dropped the WINDOW clause, our follow up tests would not have caught it, and the follow up tests were reverted?

[agavra]

@apurvam brilliantly put, patently depressing.

[agavra]

why are we only quoting reserved here and not elsewhere? why not just do it in formatIdentifier?

[agavra]

see #3222

[spena]

I'll close this PR as this code was integrated into #3222

[kekazoh]

I'm facing the same issue in a production environment (we cannot update to a beta version). Any workaround to fix this without updating?

[spena]

@kekazoh Sadly not. I tried finding workarounds when I found it, but I couldn't find any except not using reserved words in your schema.

[kekazoh]

We finally solved it creating a stream first (renaming the reserved ones).

Then we do an Insert into using aliases when selecting from the original stream.

It works like a charm!