On Mon, 16 Dec 2024 at 21:09, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Hm. If we made this behavior type-specific then you could have what
> you want without having to break any existing expectations at all.
Yeah, I agree that's definitely the safest. So, that's basically
option 2 I described in my first email, right?
Then once implemented, we might choose that in a separate patch we
want to use that hook for the json subscript.
> However, after thinking a little longer I seem to recall that we've
> previously looked into the idea of deriving the default aliases from
> the post-parse-analysis tree. We gave up because there were too many
> cases where the behavior would change, or at least it looked unduly
> painful to prevent that. For instance, something that looks like a
> function call in the raw tree could parse to half a dozen different
> kinds of nodes. So I'm not sure how we get there from here.
I think you remember wrong, or things have changed drastically since.
Because it only required fairly minimal changes to base the column
name on the transformed expression, see the attached POC. This doesn't
implement any hooks yet, but I put some comments where they could be
added. This gets the explicit cast support for free btw, because the
transform step removes that for the text type.
From 133ee1f88ec2ca732bfc6084e99a7f7d4a2710c5 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <github-t...@jeltef.nl>
Date: Mon, 16 Dec 2024 14:33:46 +0100
Subject: [PATCH v2] Use last string-literal subscript as default column name
---
src/backend/parser/parse_clause.c | 4 +-
src/backend/parser/parse_expr.c | 2 +-
src/backend/parser/parse_target.c | 128 +++++++++++++++++++++++++----
src/backend/parser/parse_utilcmd.c | 10 ++-
src/include/parser/parse_target.h | 4 +-
5 files changed, 123 insertions(+), 25 deletions(-)
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 979926b605..8008e51a2f 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -571,7 +571,7 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
funcexprs = lappend(funcexprs, newfexpr);
funcnames = lappend(funcnames,
- FigureColname((Node *) newfc));
+ FigureColname((Node *) newfc, (Expr *) newfexpr));
/* coldeflist is empty, so no error is possible */
@@ -599,7 +599,7 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
funcexprs = lappend(funcexprs, newfexpr);
funcnames = lappend(funcnames,
- FigureColname(fexpr));
+ FigureColname(fexpr, (Expr *) newfexpr));
if (coldeflist && r->coldeflist)
ereport(ERROR,
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c2806297aa..cdc0bf229f 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2377,7 +2377,7 @@ transformXmlExpr(ParseState *pstate, XmlExpr *x)
if (r->name)
argname = map_sql_identifier_to_xml_name(r->name, false, false);
else if (IsA(r->val, ColumnRef))
- argname = map_sql_identifier_to_xml_name(FigureColname(r->val),
+ argname = map_sql_identifier_to_xml_name(FigureColname(r->val, (Expr *) expr),
true, false);
else
{
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 76bf88c3ca..10996e0083 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -55,7 +55,8 @@ static List *ExpandSingleTable(ParseState *pstate, ParseNamespaceItem *nsitem,
bool make_target_entry);
static List *ExpandRowReference(ParseState *pstate, Node *expr,
bool make_target_entry);
-static int FigureColnameInternal(Node *node, char **name);
+static char *FigureColnameTransformed(Expr *expr);
+static int FigureColnameUntransformed(Node *node, char **name);
/*
@@ -99,7 +100,7 @@ transformTargetEntry(ParseState *pstate,
* Generate a suitable column name for a column without any explicit
* 'AS ColumnName' clause.
*/
- colname = FigureColname(node);
+ colname = FigureColname(node, (Expr *) expr);
}
return makeTargetEntry((Expr *) expr,
@@ -1706,15 +1707,25 @@ expandRecordVariable(ParseState *pstate, Var *var, int levelsup)
* list, we have to guess a suitable name. The SQL spec provides some
* guidance, but not much...
*
- * Note that the argument is the *untransformed* parse tree for the target
- * item. This is a shade easier to work with than the transformed tree.
+ * Note that this requires both the *untransformed* and the *transformed* parse
+ * tree. The untransformed is generally easier to work with than the
+ * transformed one, but in some cases we need the details.
*/
char *
-FigureColname(Node *node)
+FigureColname(Node *untransformed, Expr *transformed)
{
char *name = NULL;
- (void) FigureColnameInternal(node, &name);
+ /*
+ * XXX: Could add hook here that receives both transform and untransformed
+ * to allow extensions to create arbitrary column names
+ */
+
+ name = FigureColnameTransformed(transformed);
+ if (name != NULL)
+ return name;
+
+ (void) FigureColnameUntransformed(untransformed, &name);
if (name != NULL)
return name;
/* default result if we can't guess anything */
@@ -1729,16 +1740,100 @@ FigureColname(Node *node)
* we can't pick a good name.
*/
char *
-FigureIndexColname(Node *node)
+FigureIndexColname(Node *untransformed, Expr *transformed)
{
char *name = NULL;
- (void) FigureColnameInternal(node, &name);
+ /*
+ * XXX: Could add hook here to allow extensions to create arbitrary column
+ * names
+ */
+
+ name = FigureColnameTransformed(transformed);
+ if (name != NULL)
+ return name;
+
+ (void) FigureColnameUntransformed(untransformed, &name);
return name;
}
+char *
+FigureColnameTransformed(Expr *expr)
+{
+ if (expr == NULL)
+ return NULL;
+
+ switch (nodeTag(expr))
+ {
+ case T_SubscriptingRef:
+ {
+ SubscriptingRef *sbsref = (SubscriptingRef *) expr;
+ char *name = NULL;
+
+ /*
+ * XXX: Could add a hook here based on the type of
+ * refcontainertype and getSubscriptingRoutines.
+ */
+
+ if (sbsref->reflowerindexpr != NIL)
+ return NULL;
+
+ foreach_ptr(Node, upper, sbsref->refupperindexpr)
+ {
+ Const *constExpr;
+
+ if (!IsA(upper, Const))
+ continue;
+ constExpr = (Const *) upper;
+
+ if (constExpr->constisnull)
+ return NULL;
+
+ if (constExpr->consttype != TEXTOID)
+ return NULL;
+
+ name = TextDatumGetCString(constExpr->constvalue);
+ }
+ return name;
+
+ }
+ break;
+ case T_CoerceViaIO:
+ {
+ CoerceViaIO *cvi = (CoerceViaIO *) expr;
+
+ return FigureColnameTransformed(cvi->arg);
+ }
+ break;
+ case T_RelabelType:
+ {
+ RelabelType *cvi = (RelabelType *) expr;
+
+ return FigureColnameTransformed(cvi->arg);
+ }
+ break;
+ case T_ArrayCoerceExpr:
+ {
+ ArrayCoerceExpr *cvi = (ArrayCoerceExpr *) expr;
+
+ return FigureColnameTransformed(cvi->arg);
+ }
+ break;
+ case T_ConvertRowtypeExpr:
+ {
+ ConvertRowtypeExpr *cvi = (ConvertRowtypeExpr *) expr;
+
+ return FigureColnameTransformed(cvi->arg);
+ }
+ break;
+ default:
+ return NULL;
+ }
+ return NULL;
+}
+
/*
- * FigureColnameInternal -
+ * FigureColnameUntransformed -
* internal workhorse for FigureColname
*
* Return value indicates strength of confidence in result:
@@ -1749,7 +1844,7 @@ FigureIndexColname(Node *node)
* If the result isn't zero, *name is set to the chosen name.
*/
static int
-FigureColnameInternal(Node *node, char **name)
+FigureColnameUntransformed(Node *node, char **name)
{
int strength = 0;
@@ -1791,13 +1886,14 @@ FigureColnameInternal(Node *node, char **name)
if (IsA(i, String))
fname = strVal(i);
+
}
if (fname)
{
*name = fname;
return 2;
}
- return FigureColnameInternal(ind->arg, name);
+ return FigureColnameUntransformed(ind->arg, name);
}
break;
case T_FuncCall:
@@ -1812,8 +1908,8 @@ FigureColnameInternal(Node *node, char **name)
}
break;
case T_TypeCast:
- strength = FigureColnameInternal(((TypeCast *) node)->arg,
- name);
+ strength = FigureColnameUntransformed(((TypeCast *) node)->arg,
+ name);
if (strength <= 1)
{
if (((TypeCast *) node)->typeName != NULL)
@@ -1824,7 +1920,7 @@ FigureColnameInternal(Node *node, char **name)
}
break;
case T_CollateClause:
- return FigureColnameInternal(((CollateClause *) node)->arg, name);
+ return FigureColnameUntransformed(((CollateClause *) node)->arg, name);
case T_GroupingFunc:
/* make GROUPING() act like a regular function */
*name = "grouping";
@@ -1877,8 +1973,8 @@ FigureColnameInternal(Node *node, char **name)
}
break;
case T_CaseExpr:
- strength = FigureColnameInternal((Node *) ((CaseExpr *) node)->defresult,
- name);
+ strength = FigureColnameUntransformed((Node *) ((CaseExpr *) node)->defresult,
+ name);
if (strength <= 1)
{
*name = "case";
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0f324ee4e3..dc947d6590 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3059,13 +3059,15 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString)
if (ielem->expr)
{
+ /* Now do parse transformation of the expression */
+ Node *transformed = transformExpr(pstate, ielem->expr,
+ EXPR_KIND_INDEX_EXPRESSION);
+
/* Extract preliminary index col name before transforming expr */
if (ielem->indexcolname == NULL)
- ielem->indexcolname = FigureIndexColname(ielem->expr);
+ ielem->indexcolname = FigureIndexColname(ielem->expr, (Expr *) transformed);
- /* Now do parse transformation of the expression */
- ielem->expr = transformExpr(pstate, ielem->expr,
- EXPR_KIND_INDEX_EXPRESSION);
+ ielem->expr = transformed;
/* We have to fix its collations too */
assign_expr_collations(pstate, ielem->expr);
diff --git a/src/include/parser/parse_target.h b/src/include/parser/parse_target.h
index 3281f63494..5d9f670549 100644
--- a/src/include/parser/parse_target.h
+++ b/src/include/parser/parse_target.h
@@ -52,7 +52,7 @@ extern List *checkInsertTargets(ParseState *pstate, List *cols,
List **attrnos);
extern TupleDesc expandRecordVariable(ParseState *pstate, Var *var,
int levelsup);
-extern char *FigureColname(Node *node);
-extern char *FigureIndexColname(Node *node);
+extern char *FigureColname(Node *untransformed, Expr *transformed);
+extern char *FigureIndexColname(Node *untransformed, Expr *transformed);
#endif /* PARSE_TARGET_H */
base-commit: 39240bcad56dc51a7896d04a1e066efcf988b58f
--
2.43.0
