That looks like a good change. I wonder what motivates that now? Why
wasn't it added when the usages grew? Are there more Boolean usages
planned?
I ask because this code change will affect ability to automatically
cherry-pick some of the patches.
defGetBoolean() - please update the comment in the default to case to
mention defGetString along with opt_boolean_or_string production.
Reading the existing code in that function, one would wonder why to
use true and false over say on and off. But true/false seems a natural
choice. So that's fine.
defGetBoolean() and nodeRead() could use a common function to parse a
boolean string. The code in nodeRead() seems to assume that any string
starting with "t" will represent value true. Is that right?
We are using literal constants "true"/"false" at many places. This
patch adds another one. I am wondering whether it makes sense to add
#define TRUE_STR, FALSE_STR and use it everywhere for consistency and
correctness.
For the sake of consistency (again :)), we should have a function to
return string representation of a Boolean node and use it in both
defGetString and _outBoolean().
Are the expected output changes like below necessary? Might affect
backward compatibility for applications.
-bool
-----
-t
+?column?
+--------
+t
On Mon, Dec 27, 2021 at 2:32 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
>
> This patch adds a new node type Boolean, to go alongside the "value"
> nodes Integer, Float, String, etc. This seems appropriate given that
> Boolean values are a fundamental part of the system and are used a lot.
>
> Before, SQL-level Boolean constants were represented by a string with
> a cast, and internal Boolean values in DDL commands were usually
> represented by Integer nodes. This takes the place of both of these
> uses, making the intent clearer and having some amount of type safety.
--
Best Wishes,
Ashutosh Bapat