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

Invalid CQL Parsing when missing comma in List construction #1254

Closed
MikeRileyGTRI opened this issue Oct 20, 2023 · 5 comments · Fixed by #1277
Closed

Invalid CQL Parsing when missing comma in List construction #1254

MikeRileyGTRI opened this issue Oct 20, 2023 · 5 comments · Fixed by #1277
Assignees
Labels

Comments

@MikeRileyGTRI
Copy link

I believe this only happens with List types getting concatenated together in the expression. See this example below, it only seems to happen with List types. Issue is with "ListTest6"

// "TreatmentConcept_X" Concepts described above
define "TreatmentMR_0": [MedicationRequest: "TreatmentConcept_0"]
define "TreatmentMR_1": [MedicationRequest: "TreatmentConcept_1"]
define "TreatmentMR_2": [MedicationRequest: "TreatmentConcept_2"]
define "TreatmentMR_3": [MedicationRequest: "TreatmentConcept_3"]
define "TreatmentMR_4": [MedicationRequest: "TreatmentConcept_4"]

define "ListTest1": {1,2,3,4,5} //Valid
define "ListTest2": {1,2,3,4,5,} //Invalid, errant comma
define "ListTest3": {1,2,3 4,5} //Invalid, missing comma
define "ListTest4": {'A','B','C','D','E'} //Valid
define "ListTest5": {'A','B','C' 'D','E'} //Invalid missing comma
define "ListTest6": {"TreatmentMR_0", "TreatmentMR_1", "TreatmentMR_2", "TreatmentMR_3" "TreatmentMR_4"} //Valid?? Missing comma
define "ListTest7": {"TreatmentMR_0", "TreatmentMR_1", "TreatmentMR_2", "TreatmentMR_3", "TreatmentMR_4",} //Invalid, errant comma
@c-schuler c-schuler self-assigned this Oct 23, 2023
@JPercival JPercival added the bug label Oct 23, 2023
@ddieppois
Copy link

I can confirm that it seems that the translator needs a correct syntax for the list definition. If I try with the given examples I get a set of errors related to the incorrect lists. If I remove or correct them it goes through without triggering those errors, just a few related to TreatmentConcept_X.

With wrong syntax for lists:
Error:[22:31, 22:31] Syntax error at } Error:[23:18, 23:18] Syntax error at : Error:[23:22, 23:22] Syntax error at , Error:[23:24, 23:24] Syntax error at , Error:[23:28, 23:28] Syntax error at , Error:[23:30, 23:30] Syntax error at } Error:[24:18, 24:18] Syntax error at : Error:[25:33, 25:33] Syntax error at 'D' Error:[27:105, 27:105] Syntax error at }

Nothing when removed.

@MikeRileyGTRI
Copy link
Author

MikeRileyGTRI commented Oct 24, 2023

It would be helpful if we could see the ANTLR parse tree from the script, but I don't know how to do that (would be nice if the CQL dev guide put out some instructions on using ANTLR tools to help debug scripts). My intuition is that the 2 definition identifiers right next to each other is counting as a singular 'expression' from parsing, because only the first definitions of the results in the "bad concatenated set" was captured and rendered when the script ran.

Here's the list specification from the ANTLR rules.

listSelector
    : ('List' ('<' typeSpecifier '>')?)? '{' (expression (',' expression)*)? '}'
    ;

Clearly; the only thing that 2 quote strings with whitespace between them could be seen as is an 'expression' right?

@MikeRileyGTRI MikeRileyGTRI changed the title Incorrect CQL Parsing when missing comma in List construction Invalid CQL Parsing when missing comma in List construction Oct 24, 2023
@JPercival
Copy link
Contributor

I was mistaken on Zulip. I took a look at this and it is indeed valid CQL. The second identifier is treated as an alias, as if you were querying a list. This is a result of automatic List Promotion

library Test

define "1": 1
define "2": 2
define "3": 3
define "4": 4
define "5": 5

define "Four With Five": "4" "5" // "5" is interpreted as an alias here, result is 4
define "Query Four": ({ "4" }) "X" return "X" //  This is how the above expression looks to the CQL compiler

define "Four With Alias": "4" "15" return Add("15", 1) // "15" doesn't exist, It's a alias for "4". result is 5 (4+1)

define "ListTest6": {"1", "2", "3", "4" "5"} // Valid, "5" is an alias for "4"

There's some work here to add warnings for hidden identifiers which will help identify this issue:
#671

I'll prioritize reviewing that and getting it in.

@MikeRileyGTRI
Copy link
Author

Why are quoted identifiers allowed for aliases? No examples in the cookbook, the author's guide, or the developer's guide use quoted identifiers. This feels to me like a feature of the grammar that's overly permissive. Especially in List/Tuple declaration, it's would mostly pretty niche to want to define items with aliases within a List or Tuple. And such definitions would be difficult to parse/unintuitive scripting.

@JPercival
Copy link
Contributor

Any identifier is allowed as an alias for the same reason quoted identifiers are allowed in general, which is that it gives an author a way to assign a “natural language” meaning, including spaces and so on. Deeply nested/chained queries could have aliases like “Patient Encounter With Correlated Cancer Diagnosis”. That said, I agree that’s it’s confusing in this case. :)

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.

5 participants