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

non-cosmetic difference detected if semicolons present #4

Closed
djrobstep opened this issue Mar 24, 2018 · 5 comments
Closed

non-cosmetic difference detected if semicolons present #4

djrobstep opened this issue Mar 24, 2018 · 5 comments

Comments

@djrobstep
Copy link

djrobstep commented Mar 24, 2018

I get a non-cosmetic difference detected when I try and use pgpp on a file with eg the following contents:

select * from t;

The same query, and other queries are fine without the semicolon.

What's happening here? Perhaps pgpp doesn't support multiple statements?

Happy to take a look at fixing if you'd like to point me in the right direction?

@lelit
Copy link
Owner

lelit commented Mar 25, 2018

You can see what's happening by looking at pgpp -t output for the two statements, and spotting the difference: this is basically what the safety belt does, ie it compares the AST of the input statement with that of the prettified one, and if there is any discrepancy, it raises an exception.
A quick test revealed that, when there is a semicolon, the underlying PG parser emits a stmt_len slot, not present otherwise:

$ diff -u <(echo "select * from i" | pgpp -t) <(echo "select * from i;" | pgpp -t)
--- /dev/fd/63	2018-03-25 10:51:54.356248860 +0200
+++ /dev/fd/62	2018-03-25 10:51:54.356248860 +0200
@@ -32,7 +32,8 @@
             }
           ]
         }
-      }
+      },
+      "stmt_len": 15
     }
   }
 ]

I'll try to understand the meaning of that stmt_len: AFAICT, it should be ignored when comparing the ASTs.

Thank you for reporting the issue!

@lelit
Copy link
Owner

lelit commented Mar 25, 2018

Here there is a brief explanation of the meaning of stmt_len.

Also libpg_query seems to explicitly ignore it to compute statement's fingerprint.

So yes, I think it's just a matter of removing it from the structure before comparison, as we already do for the location field.

@lfittl
Copy link

lfittl commented Mar 26, 2018

@lelit Just in case it helps, here is the full reference for the fingerprinting logic in pg_query itself: https://github.com/lfittl/libpg_query/wiki/Fingerprinting#version-20-based-on-postgresql-10

(the stmt_len is a Postgres 10 addition, and if I understand right I would ignore it for any comparisons between ASTs in your library as well)

@lelit
Copy link
Owner

lelit commented Mar 26, 2018

Thanks Lukas for the confirmation!

lelit added a commit that referenced this issue Mar 27, 2018
@lelit
Copy link
Owner

lelit commented Mar 31, 2018

I just release version 0.25 that fixes this.

@lelit lelit closed this as completed Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants