korbit-ai[bot] commented on code in PR #35424:
URL: https://github.com/apache/superset/pull/35424#discussion_r2395837206
##########
superset/sql/dialects/pinot.py:
##########
@@ -50,6 +51,16 @@ class Tokenizer(MySQL.Tokenizer):
"BYTES": TokenType.VARBINARY,
}
+ class Parser(MySQL.Parser):
+ FUNCTIONS = {
+ **MySQL.Parser.FUNCTIONS,
+ "DATE_ADD": lambda args: exp.DateAdd(
+ this=seq_get(args, 2),
+ expression=seq_get(args, 1),
+ unit=seq_get(args, 0),
+ ),
+ }
Review Comment:
### Fragile positional argument handling in DATE_ADD function
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The DATE_ADD function implementation uses positional arguments with seq_get
which makes the code fragile and harder to understand.
###### Why this matters
Using positional arguments with seq_get makes the code more prone to errors
if the argument order changes and reduces code readability since the meaning of
each position is not immediately clear without documentation.
###### Suggested change ∙ *Feature Preview*
Consider creating a named tuple or dataclass to represent the DATE_ADD
arguments, or use more descriptive variable names with proper validation:
```python
def parse_date_add_args(args):
if len(args) != 3:
raise ValueError("DATE_ADD requires exactly 3 arguments")
unit, expression, date = args
return exp.DateAdd(
this=date,
expression=expression,
unit=unit
)
"DATE_ADD": parse_date_add_args
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/71cc789a-28ea-4a34-8360-fcbe4386d408/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/71cc789a-28ea-4a34-8360-fcbe4386d408?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/71cc789a-28ea-4a34-8360-fcbe4386d408?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/71cc789a-28ea-4a34-8360-fcbe4386d408?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/71cc789a-28ea-4a34-8360-fcbe4386d408)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0edd3d87-666d-45bb-8144-dd7bee33b76d -->
[](0edd3d87-666d-45bb-8144-dd7bee33b76d)
##########
superset/sql/dialects/pinot.py:
##########
@@ -50,6 +51,16 @@ class Tokenizer(MySQL.Tokenizer):
"BYTES": TokenType.VARBINARY,
}
+ class Parser(MySQL.Parser):
+ FUNCTIONS = {
+ **MySQL.Parser.FUNCTIONS,
+ "DATE_ADD": lambda args: exp.DateAdd(
+ this=seq_get(args, 2),
+ expression=seq_get(args, 1),
+ unit=seq_get(args, 0),
+ ),
Review Comment:
### Incorrect argument order in DATE_ADD parser <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The DATE_ADD function parser maps arguments in the wrong order, with unit at
index 0, expression at index 1, and this at index 2, which doesn't match the
standard DATE_ADD(date, INTERVAL value unit) syntax.
###### Why this matters
This will cause incorrect parsing of DATE_ADD function calls, potentially
swapping the date expression with the interval value or unit, leading to
runtime errors or incorrect query results.
###### Suggested change ∙ *Feature Preview*
Correct the argument mapping to match the standard DATE_ADD syntax:
```python
"DATE_ADD": lambda args: exp.DateAdd(
this=seq_get(args, 0),
expression=seq_get(args, 2),
unit=seq_get(args, 1),
),
```
This assumes DATE_ADD(date, unit, value) format. Verify the actual Pinot
DATE_ADD syntax and adjust accordingly.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83648b11-fd24-4465-8289-51fed3ea1838/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83648b11-fd24-4465-8289-51fed3ea1838?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83648b11-fd24-4465-8289-51fed3ea1838?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83648b11-fd24-4465-8289-51fed3ea1838?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/83648b11-fd24-4465-8289-51fed3ea1838)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:55778ca0-f273-4872-afc6-8410f23dd502 -->
[](55778ca0-f273-4872-afc6-8410f23dd502)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
