Re: [PATCHES] transformExpr() refactor
On Wed, 2005-01-19 at 18:39 +1100, Neil Conway wrote: Attached is a revised patch. Barring any objections, I intend to apply this sometime tomorrow. Applied. -Neil ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] transformExpr() refactor
On Tue, 2005-01-18 at 00:54 -0500, Tom Lane wrote:
I won't stand in the way of you doing this
Attached is a revised patch. Barring any objections, I intend to apply
this sometime tomorrow.
-Neil
Index: src/backend/parser/parse_expr.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.179
diff -c -r1.179 parse_expr.c
*** src/backend/parser/parse_expr.c 12 Jan 2005 17:32:36 - 1.179
--- src/backend/parser/parse_expr.c 19 Jan 2005 07:07:48 -
***
*** 37,45
--- 37,63
bool Transform_null_equals = false;
+ static Node *transformParamRef(ParseState *pstate, ParamRef *pref);
+ static Node *transformAExprOp(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprAnd(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOr(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprNot(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOpAny(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOpAll(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprDistinct(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprNullIf(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOf(ParseState *pstate, A_Expr *a);
+ static Node *transformFuncCall(ParseState *pstate, FuncCall *fn);
+ static Node *transformCaseExpr(ParseState *pstate, CaseExpr *c);
+ static Node *transformSubLink(ParseState *pstate, SubLink *sublink);
+ static Node *transformArrayExpr(ParseState *pstate, ArrayExpr *a);
+ static Node *transformRowExpr(ParseState *pstate, RowExpr *r);
+ static Node *transformCoalesceExpr(ParseState *pstate, CoalesceExpr *c);
+ static Node *transformBooleanTest(ParseState *pstate, BooleanTest *b);
static Node *transformColumnRef(ParseState *pstate, ColumnRef *cref);
static Node *transformWholeRowRef(ParseState *pstate, char *schemaname,
char *relname);
+ static Node *transformBooleanTest(ParseState *pstate, BooleanTest *b);
static Node *transformIndirection(ParseState *pstate, Node *basenode,
List *indirection);
static Node *typecast_expression(ParseState *pstate, Node *expr,
***
*** 90,154
switch (nodeTag(expr))
{
case T_ColumnRef:
! {
! result = transformColumnRef(pstate, (ColumnRef *) expr);
! break;
! }
! case T_ParamRef:
! {
! ParamRef *pref = (ParamRef *) expr;
! int paramno = pref-number;
! ParseState *toppstate;
! Param *param;
!
! /*
! * Find topmost ParseState, which is where paramtype info
! * lives.
! */
! toppstate = pstate;
! while (toppstate-parentParseState != NULL)
! toppstate = toppstate-parentParseState;
! /* Check parameter number is in range */
! if (paramno = 0) /* probably can't happen? */
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_PARAMETER),
! errmsg(there is no parameter $%d, paramno)));
! if (paramno toppstate-p_numparams)
! {
! if (!toppstate-p_variableparams)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_PARAMETER),
! errmsg(there is no parameter $%d,
! paramno)));
! /* Okay to enlarge param array */
! if (toppstate-p_paramtypes)
! toppstate-p_paramtypes =
! (Oid *) repalloc(toppstate-p_paramtypes,
! paramno * sizeof(Oid));
! else
! toppstate-p_paramtypes =
! (Oid *) palloc(paramno * sizeof(Oid));
! /* Zero out the previously-unreferenced slots */
! MemSet(toppstate-p_paramtypes + toppstate-p_numparams,
! 0,
! (paramno - toppstate-p_numparams) * sizeof(Oid));
! toppstate-p_numparams = paramno;
! }
! if (toppstate-p_variableparams)
! {
! /* If not seen before, initialize to UNKNOWN type */
! if (toppstate-p_paramtypes[paramno - 1] == InvalidOid)
! toppstate-p_paramtypes[paramno - 1] = UNKNOWNOID;
! }
- param = makeNode(Param);
- param-paramkind = PARAM_NUM;
- param-paramid = (AttrNumber) paramno;
- param-paramtype = toppstate-p_paramtypes[paramno - 1];
- result = (Node *) param;
- break;
- }
case T_A_Const:
{
A_Const*con = (A_Const *) expr;
--- 108,120
switch (nodeTag(expr))
{
case T_ColumnRef:
! result = transformColumnRef(pstate, (ColumnRef *) expr);
! break;
! case T_ParamRef:
! result = transformParamRef(pstate, (ParamRef *) expr);
! break;
case T_A_Const:
{
A_Const*con = (A_Const *) expr;
***
*** 160,165
--- 126,132
con-typename);
break;
}
+
case T_A_Indirection:
{
A_Indirection *ind = (A_Indirection *) expr;
***
*** 169,174
--- 136,142
ind-indirection);
break;
}
+
case T_TypeCast:
{
TypeCast *tc = (TypeCast *) expr;
***
*** 177,182
---
Re: [PATCHES] transformExpr() refactor
On Thu, 2004-10-28 at 16:49 +1000, Neil Conway wrote:
This patch refactors transformExpr(): rather than being a monsterous 900
line function, it breaks it up into numerous sub-functions that are
invoked by transformExpr() for individual expression types, in the style
of transformStmt().
I still think this patch is worth applying. Sadly, I will need to
basically rewrite it due to code drift. I'm happy to do that, although
I'd like to resolve whether we want to accept it or not. Tom and Bruce
objected when I posted it originally, although I don't think we reached
a conclusion.
Why I think the patch is a good idea: 900 line functions are almost
universally bad (in fact, I'd be tempted to remove the almost). A 900
line function that divides distinct functionality into different
branches of a switch statement isn't _that_ evil, but it is still a
large hunk of code for someone to understand as a single, monolithic
unit. Because each branch of the switch statement is independent, we
can trivially split each branch off into a function -- each branch does
something distinct, so it ought to be a distinct function. That means
the independence of each branch of the switch statement is guaranteed
(the function can't modify a local variable in the calling function, for
example), and it means that the code is conceptually easier to read.
With the present layout,
It does mean that you'll need to jump to the function definition if you
want to see what a particular branch of the switch statement does. But
(a) use tags, it's not tough (b) this is a _good_ thing. If I want to
understand what the parent function does (transformExpr(), the one with
the switch), I don't want to have to grok through a 700 line switch
statement. If each branch of the switch just invokes a function, the
intent of transformExpr() is perfectly clear.
For refresh everyone's memory, I've attached the original version of the
patch. It won't apply cleanly to current sources, but it should apply to
HEAD as of approximately Oct. 28, 2004.
-Neil
Index: src/backend/parser/parse_expr.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.176
diff -c -r1.176 parse_expr.c
*** src/backend/parser/parse_expr.c 29 Aug 2004 05:06:44 - 1.176
--- src/backend/parser/parse_expr.c 28 Oct 2004 06:46:54 -
***
*** 37,45
--- 37,62
bool Transform_null_equals = false;
+ static Node *transformParamRef(ParseState *pstate, ParamRef *pref);
+ static Node *transformAExprOp(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprAnd(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOr(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprNot(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOpAny(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOpAll(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprDistinct(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprNullIf(ParseState *pstate, A_Expr *a);
+ static Node *transformAExprOf(ParseState *pstate, A_Expr *a);
+ static Node *transformFuncCall(ParseState *pstate, FuncCall *fn);
+ static Node *transformCaseExpr(ParseState *pstate, CaseExpr *c);
+ static Node *transformSubLink(ParseState *pstate, SubLink *sublink);
+ static Node *transformArrayExpr(ParseState *pstate, ArrayExpr *a);
+ static Node *transformRowExpr(ParseState *pstate, RowExpr *r);
+ static Node *transformCoalesceExpr(ParseState *pstate, CoalesceExpr *c);
static Node *transformColumnRef(ParseState *pstate, ColumnRef *cref);
static Node *transformWholeRowRef(ParseState *pstate, char *schemaname,
char *relname);
+ static Node *transformBooleanTest(ParseState *pstate, BooleanTest *b);
static Node *transformIndirection(ParseState *pstate, Node *basenode,
List *indirection);
static Node *typecast_expression(ParseState *pstate, Node *expr,
***
*** 90,154
switch (nodeTag(expr))
{
case T_ColumnRef:
! {
! result = transformColumnRef(pstate, (ColumnRef *) expr);
! break;
! }
case T_ParamRef:
! {
! ParamRef *pref = (ParamRef *) expr;
! int paramno = pref-number;
! ParseState *toppstate;
! Param *param;
!
! /*
! * Find topmost ParseState, which is where paramtype info
! * lives.
! */
! toppstate = pstate;
! while (toppstate-parentParseState != NULL)
! toppstate = toppstate-parentParseState;
!
! /* Check parameter number is in range */
! if (paramno = 0) /* probably can't happen? */
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_PARAMETER),
! errmsg(there is no parameter $%d, paramno)));
! if (paramno toppstate-p_numparams)
! {
! if (!toppstate-p_variableparams)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_PARAMETER),
! errmsg(there is no parameter $%d,
! paramno)));
!
Re: [PATCHES] transformExpr() refactor
Neil Conway [EMAIL PROTECTED] writes: Why I think the patch is a good idea: 900 line functions are almost universally bad (in fact, I'd be tempted to remove the almost). [ shrug... ] 900 line functions that consist of absolutely independent case arms are not any harder to read than the alternative. I won't stand in the way of you doing this, but I think you could find more profitable uses for your time. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] transformExpr() refactor
On Thu, 2004-10-28 at 18:00, Neil Conway wrote: I think the code is more readable this way. FWIW, I'm +1 on the patch for the above reason. -- Regards, James William Pye signature.asc Description: This is a digitally signed message part
Re: [PATCHES] transformExpr() refactor
James William Pye wrote: -- Start of PGP signed section. On Thu, 2004-10-28 at 18:00, Neil Conway wrote: I think the code is more readable this way. FWIW, I'm +1 on the patch for the above reason. I liked the large case statement myself. I don't like breaking things into pieces when the pieces are just splits out of a switch structure. You are basically giving names to switch actions and pushing into functions with names. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] transformExpr() refactor
Neil Conway [EMAIL PROTECTED] writes: This patch refactors transformExpr(): rather than being a monsterous 900 line function, it breaks it up into numerous sub-functions that are invoked by transformExpr() for individual expression types, in the style of transformStmt(). I don't actually find this to be an improvement. What's the point? Since all the switch arms are independent, you haven't really done anything at all to improve the comprehensibility of the code... regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] transformExpr() refactor
On Fri, 2004-10-29 at 00:17, Tom Lane wrote: I don't actually find this to be an improvement. What's the point? Since all the switch arms are independent, you haven't really done anything at all to improve the comprehensibility of the code... I think the code is more readable this way. The very fact that the switch branches are completely independent is good enough reason to make them distinct functions, IMHO. Breaking 900 lines of code into smaller chunks of code, each of which does a single conceptual task, just makes the whole enterprise easier to understand. As for sharing code between the functions, I agree that isn't done today -- but it will be easier to do in the future with this refactoring. -Neil ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
