nene / prettier-plugin-sql-cst

Prettier SQL plugin that uses sql-parser-cst
https://nene.github.io/prettier-sql-playground/
GNU General Public License v3.0
123 stars 7 forks source link

Postgres COALESCE requires parens #28

Closed maheshsundaram closed 1 week ago

maheshsundaram commented 5 months ago

Hi! This might be a limitation of sql-parser-cst? I would be glad to help out if you can point me in the right direction on how to begin.

The issue here is that the parens in the inner select statement for the first parameter to coalesce are required, but this plugin removes them.

CREATE TABLE table_name (
  col1 TEXT,
  col2 TEXT,
  col3 TEXT
);

INSERT INTO table_name (col1, col2, col3) VALUES ('one', 'two', 'three');

CREATE TABLE values_table (
  value TEXT
);

INSERT INTO table_name (col1, col2, col3)
SELECT
  'val1',
  'val2',
  COALESCE(
    (SELECT value FROM values_table WHERE value = 'not found'), -- Parens are required here
    'default_value'
  );

INSERT INTO table_name (col1, col2, col3)
SELECT
  'val1',
  'val2',
  COALESCE(
    SELECT value FROM values_table WHERE value = 'not found', -- sql-cst plugin removes them
    'default_value'
  );
nene commented 5 months ago

Thanks for reporting. It's a bug in the pretty-printing code, not in the parser.

This should be fairly simple to fix. Take a look at: https://github.com/nene/prettier-plugin-sql-cst/blob/master/src/syntax/expr.ts#L43-L58

Currently the code removes all parenthesis around function arguments. But that's apparently too aggressive. We should not do it when the argument is a select statement.

maheshsundaram commented 4 months ago

Thanks for your reply and for highlighting that block. Is there an "isSelect"? From reading that code it looks like something like this is needed?

  paren_expr: (print, node, path) => {
    // discard unnecessary nested ((parentheses))
    if (isParenExpr(node.expr)) {
      return print("expr");
    }
    // Discard unnecessary parenthesis around function arguments
    if (
      isListExpr(path.getParentNode(0)) &&
      isFuncArgs(path.getParentNode(1)) &&
      !isSelect(path.getParentNode(1))
    ) {
      return print("expr");
    }
    // ...
nene commented 4 months ago

There is no isSelect, but that should be trivial to implement. You want to though cover both plain type:"select_stmt" and also type:"compound_select_stmt" (the latter is used for e.g. UNION of two selects).

!isSelect(path.getParentNode(1))

Here you's checking that the grandparent node is not a select, which it never will be because the previous line already checks that it is in fact a function arguments list. What you instead want to check is that the child expression of paren_expr is not a select:

!isSelect(node.expr)
nene commented 4 months ago

You can use sql-explorer to see what the syntax tree looks like.

bparnell-4tel commented 1 week ago

You should be right to close this now.