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

After update 4.1.2 -> 4.2.0 arithmetic operations in brackets are comma separated #299

Open
f1r3starter opened this issue Dec 28, 2018 · 3 comments

Comments

@f1r3starter
Copy link

Good day, after recent updated from 4.1.2 -> 4.2.0 queries like
SELECT ( table1.value1 + COALESCE(table1.value2, 0) ) AS total_value

are being parsed with comma(,) delimiter , which means that after recreation it looks like:
SELECT ( table1.value1,+,COALESCE(table1.value2, 0) ) AS total_value

so arithmetic operations are comma separated.

@nicoder
Copy link
Contributor

nicoder commented Dec 28, 2018

Hi @f1r3starter,

I haven't checked yet but it's probably due to the merging of this PR : #295

(if you need to revert those changes temporarily)

I'll try to see if I can understand the code better and fix it.

@f1r3starter
Copy link
Author

@nicoder , thanks for reply!
I reverted version, but I think it is a bug, after short debugging I found out that comma is concatenated on this line, I guess it is somehow related to SignBuilder

nicoder added a commit to nicoder/PHP-SQL-Parser that referenced this issue May 20, 2019
…separated

for example the query:

```sql
SELECT (1 + 2) AS THREE, (1 + 3) AS FOUR
```

would become:

```sql
SELECT (1,+,2) AS THREE, (1 + 3) AS FOUR
```

This is a regression caused by commit 3a9d906, that meant to add missing
commas in expressions like `CAST(... AS DECIMAL(16, 2))`,
but added too many commas visibly.

=> revert the parts of that commit that changed the code (leave the
added test assertion)

=> change the `ExpressionListProcessor` code that handles
unspecified nodes.

That code was added in commit 63d59d7, apparently to solve issue 51:
https://code.google.com/archive/p/php-sql-parser/issues/51

It seems to have copied and pasted the code that handles function
arguments, just above.

But that code does not keep commas,
and while the `FunctionBuilder` adds them back,
the `SelectBracketExpressionBuilder` does not.

The issue51Test test seems to cover the bug described in issue 51,
and it still passes.

So maybe we do not need the same handling as for function arguments in
unspecified nodes?

Hopefully someone with a better understanding of this code can have a look.
@nicoder
Copy link
Contributor

nicoder commented May 20, 2019

I opened a PR : #306 to try and fix this.

Hopefully someone with a better understanding of that code can have a look.

greenlion pushed a commit that referenced this issue Jun 1, 2019
…#306)

for example the query:

```sql
SELECT (1 + 2) AS THREE, (1 + 3) AS FOUR
```

would become:

```sql
SELECT (1,+,2) AS THREE, (1 + 3) AS FOUR
```

This is a regression caused by commit 3a9d906, that meant to add missing
commas in expressions like `CAST(... AS DECIMAL(16, 2))`,
but added too many commas visibly.

=> revert the parts of that commit that changed the code (leave the
added test assertion)

=> change the `ExpressionListProcessor` code that handles
unspecified nodes.

That code was added in commit 63d59d7, apparently to solve issue 51:
https://code.google.com/archive/p/php-sql-parser/issues/51

It seems to have copied and pasted the code that handles function
arguments, just above.

But that code does not keep commas,
and while the `FunctionBuilder` adds them back,
the `SelectBracketExpressionBuilder` does not.

The issue51Test test seems to cover the bug described in issue 51,
and it still passes.

So maybe we do not need the same handling as for function arguments in
unspecified nodes?

Hopefully someone with a better understanding of this code can have a look.
theilig pushed a commit to theilig/PHP-SQL-Parser that referenced this issue Jan 15, 2021
…separated (greenlion#306)

for example the query:

```sql
SELECT (1 + 2) AS THREE, (1 + 3) AS FOUR
```

would become:

```sql
SELECT (1,+,2) AS THREE, (1 + 3) AS FOUR
```

This is a regression caused by commit 3a9d906, that meant to add missing
commas in expressions like `CAST(... AS DECIMAL(16, 2))`,
but added too many commas visibly.

=> revert the parts of that commit that changed the code (leave the
added test assertion)

=> change the `ExpressionListProcessor` code that handles
unspecified nodes.

That code was added in commit 63d59d7, apparently to solve issue 51:
https://code.google.com/archive/p/php-sql-parser/issues/51

It seems to have copied and pasted the code that handles function
arguments, just above.

But that code does not keep commas,
and while the `FunctionBuilder` adds them back,
the `SelectBracketExpressionBuilder` does not.

The issue51Test test seems to cover the bug described in issue 51,
and it still passes.

So maybe we do not need the same handling as for function arguments in
unspecified nodes?

Hopefully someone with a better understanding of this code can have a look.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants