NULL results in SQL (Re: sql_null in Roxen 5.0)

[Stephen R. van den Berg]

Martin Stjernholm wrote:
>"Stephen R. van den Berg" <srb@...> wrote:
>> Pike 7.8 returns Pike UNDEFINED objects in Sql.Sql queries for columns
>> that return NULL values (as opposed to integer zero); Pike <7.8 returned
>> integer zeros.

>On the pike level the interface isn't ambiguous as it was on the RXML
>level, since all other values are returned as strings. It's therefore
>not a problem returning either a normal zero or UNDEFINED for an sql
>NULL.

>Therefore I don't really see the point with the change in 7.8. Note
>also that a mapping cannot carry UNDEFINED as a value for natural
>reasons, so in Sql.query() those UNDEFINED's still becomes normal
>zeroes.

Aaargh.  Didn't know that.

>The fact that all non-NULL values are returned as strings is another
>problem in the Sql module, but it does avoid this particular problem.
>If a better API is added to it, where integers are returned as ints
>etc, then there should be an SqlNull object there too for sql NULLs.
>Using UNDEFINED for it would be abuse (see below). (Further
>discussions about this should go to the pike dev list.)

Well, actually, a cursory check amongst database backends in the
current (and older) Pike versions shows:

               Returns

- Mysql       string/float/int
- Oracle       string/float/int
- pgsql       string/float/int
- Odbc       string/wide-string
- Postgres     string

So the current backend database API actually desires native types.
I.e. using UNDEFINED for NULL there is desirable (because queries to
Mysql/Oracle/pgsql cannot distinguish NULL and integer-zero values now)
and works.

>Those changes shouldn't be necessary since all non-null values are
>strings. If your sql glue does something else then it doesn't adhere
>to the Sql module API.

Well, actually it does, since I copied the behaviour from the Mysql
low-level driver.

>Btw, I wouldn't call the UNDEFINED value in pike an object. It's not,
>and it'd be a bit misleading to think of it as one.

Well, in light of learning that mappings cannot contain UNDEFINEDs, I
propose to introduce the following SqlNULL() type which should be returned
by any mappingvalues from Sql.Sql()->query() which are actually an SQL
NULL type.

Any thoughts?  If there are no objections I'd like to get it into 7.8.
The SqlNULL type is carefully tuned to be transparent for most normal uses
(casts, comparisons, printing and truth-values).

commit b68690a72d6855ecc187a4be35037cb080db69d4
Author: Stephen R. van den Berg <srb@...>
Date:   Fri Aug 22 13:01:00 2008 +0200

    Sql: Use SqlNULL to represent NULL values returned from Sql.query()

diff --git a/CHANGES b/CHANGES
index f1c8f6f..060b5db 100644
--- a/CHANGES
+++ b/CHANGES
@@ -691,6 +691,8 @@ o Regexp.PCRE.Widestring

 o Sql

+  - Return an SqlNULL() object for NULL values in Sql.Sql()->query() results
+
   - Bugfixes in listing Postgres fields.

   - If ENABLE_SPAWN_RSQLD is defined, rsqld will be spawned when
diff --git a/lib/modules/Sql.pmod/Sql.pike b/lib/modules/Sql.pmod/Sql.pike
index 3ec85d5..98a7dac 100644
--- a/lib/modules/Sql.pmod/Sql.pike
+++ b/lib/modules/Sql.pmod/Sql.pike
@@ -339,6 +339,16 @@ protected string _sprintf(int type, mapping|void flags)
     return sprintf("Sql.%O", master_sql);

+// Actually the following object is a constant and needs only
+// one instance per Pike runtime
+.sql_util.SqlNULL SqlNULL = .sql_util.SqlNULL();
+
+final protected void replace_SqlNULL(array(mixed) row) {
+  foreach(row;int i;mixed value)
+    if(zero_type(value))
+      row[i] = SqlNULL;
+}
+
 protected array(mapping(string:mixed)) res_obj_to_array(object res_obj)
 {
   if (res_obj)
@@ -366,11 +376,15 @@ protected array(mapping(string:mixed)) res_obj_to_array(object res_obj)
       fieldnames = map(fieldnames, lower_case);

     if(has_table)
-      while (row = res_obj->fetch_row())
+      while (row = res_obj->fetch_row()) {
+ replace_SqlNULL(row);
  res += ({ mkmapping(fieldnames, row + row) });
+      }
     else
-      while (row = res_obj->fetch_row())
+      while (row = res_obj->fetch_row()) {
+ replace_SqlNULL(row);
  res += ({ mkmapping(fieldnames, row) });
+      }

     return res;

diff --git a/lib/modules/Sql.pmod/sql_util.pmod b/lib/modules/Sql.pmod/sql_util.pmod
index 9a0bb92..482538d 100644
--- a/lib/modules/Sql.pmod/sql_util.pmod
+++ b/lib/modules/Sql.pmod/sql_util.pmod
@@ -207,3 +207,61 @@ class MySQLBrokenUnicodeWrapper

 #endif
+
+class SqlNULL
+{
+  constant is_SqlNULL = 1;
+
+  // Treat these objects as indistinguishable from each other. We
+  // ought to ensure that there's only one in the pike process
+  // instead, but that's tricky to solve in the PCode codec.
+  protected int `== (mixed other) {
+    return zero_type(other)
+     || objectp(other) && other->is_SqlNULL;
+  }
+
+  protected int `! () {
+    return 1;
+  }
+
+  protected int(0..1) _is_type(mixed type) {
+    return 1;
+  }
+
+  protected mixed cast(string to) {
+    mixed ret=UNDEFINED;
+    switch(to) {
+      case "string":
+ ret="";
+ break;
+      case "float":
+ ret=Math.nan;
+ break;
+      case "mixed":case "object":
+ ret=this;
+    }
+    return ret;
+  }
+
+  protected int __hash() {
+    return 17;
+  }
+
+  protected string _sprintf (int flag) {
+    string ret=UNDEFINED;
+    switch(flag) {
+      case 'O':ret="SqlNULL()";
+        break;
+      case 's':case 'd':case 'f':case 'g':ret="";
+        break;
+    }
+    return ret;
+  }
+
+  int _encode() {
+    return 0;
+  }
+
+  void _decode (int dummy) {
+  }
+}
--
Sincerely,
           Stephen R. van den Berg.

"If you try to fail and succeed, which have you done?"

[Martin Bähr]

On Fri, Aug 22, 2008 at 01:05:32PM +0200, Stephen R. van den Berg wrote:
> Well, in light of learning that mappings cannot contain UNDEFINEDs, I
> propose to introduce the following SqlNULL() type which should be returned
> by any mappingvalues from Sql.Sql()->query() which are actually an SQL
> NULL type.
>
> Any thoughts?  If there are no objections I'd like to get it into 7.8.

introduction of a real NULL type has had a lot of discussion without any
solution (did you miss that? :-)

and while i welcome this change i think it should get a lot of testing
to make sure no code is negatively affected.

greetings, martin.

1. Keep this out of 7.8.

2. I'd much rather see Sql.NULL, with as little fuss as possible e.g.

Sql.pmod/module.pmod:

object NULL = Pike.Null();


Pike.pmod/module.pmod:

class Null
{
  constant is_null = 1;

  int _encode() { return 0; }
  void _decode(int zero) { }
}

[Stephen R. van den Berg]

Martin B?hr wrote:
>On Fri, Aug 22, 2008 at 01:05:32PM +0200, Stephen R. van den Berg wrote:
>> Well, in light of learning that mappings cannot contain UNDEFINEDs, I
>> propose to introduce the following SqlNULL() type which should be returned
>> by any mappingvalues from Sql.Sql()->query() which are actually an SQL
>> NULL type.

>> Any thoughts?  If there are no objections I'd like to get it into 7.8.

>introduction of a real NULL type has had a lot of discussion without any
>solution (did you miss that? :-)

I vaguely recall that, yes, but I'm not trying to solve the generic
case, just the SQL-case.

>and while i welcome this change i think it should get a lot of testing
>to make sure no code is negatively affected.

Obviously.  Then again the proposed definition has some special
properties which make it almost transparent in a lot of existing code.
--
Sincerely,
           Stephen R. van den Berg.

"If you try to fail and succeed, which have you done?"

[Stephen R. van den Berg]

Martin Nilsson (Opera Mini - AFK!) @ Pike (-) developers forum wrote:
>2. I'd much rather see Sql.NULL, with as little fuss as possible e.g.

>class Null
>{
>  constant is_null = 1;
>  int _encode() { return 0; }
>  void _decode(int zero) { }

That results in something distinguishable, but it requires (IMHO clumsy)
special handling to process right and does not go down well with
existing code.

The properties of my proposed SqlNULL type allow it to be used almost without
thinking in existing code, and only if you really want to detect the
NULL, you are able to.

The reason I'd like to keep it mostly transparent is because it more
closely matches SQL-semantics.  I.e. SQL allows you to arbitrarily use
NULL values in all expressions and does something "sane" (depending on
your viewpoint, of course; at least it's well-defined according to the
SQL-ANSI standard) with it.  If you create an object for it that
doesn't allow even the most basic of operations on it, it will stick out
like a sore thumb anytime you need to cater for it.
--
Sincerely,
           Stephen R. van den Berg.

"If you try to fail and succeed, which have you done?"

> >Therefore I don't really see the point with the change in 7.8. Note
> >also that a mapping cannot carry UNDEFINED as a value for natural
> >reasons, so in Sql.query() those UNDEFINED's still becomes normal
> >zeroes.
>
> Aaargh.  Didn't know that.

That's the raison d'être for UNDEFINED, to be able to see if a mapping
lookup failed to find the key. Unfortunately that semantic has gotten
severely muddled since many are very anxious to use it as a sort of
NULL value (the name "UNDEFINED" is actually rather well chosen).

> > Well, actually, a cursory check amongst database backends in the
> current (and older) Pike versions shows:
>
>                Returns
>
> - Mysql       string/float/int

Your check appears to be a little too cursory; this case is string
only (since OLD_SQL_COMPAT is typically not defined). Although it's
only one backend, it's in my world the by far most important one.

> - Oracle       string/float/int

Probably true. The Oracle driver has always been a bit odd. It's
unfortunate that this semantic isn't uniform across all drivers. But
still, string only is the standard. Much of the code I'm concerned
with (e.g. Roxen) would break quite a bit if it's anything else. The
Oracle module ought to be fixed, but it requires compat of course.

> - pgsql       string/float/int

This is your new one, right? Then, I'm sorry to say, it has little
relevance in this discussion.

> Well, in light of learning that mappings cannot contain UNDEFINEDs, I
> propose to introduce the following SqlNULL() type which should be returned
> by any mappingvalues from Sql.Sql()->query() which are actually an SQL
> NULL type.

An Sql.NULL value would be good, but this would also need new API
calls, since I stubbornly maintain that query and big_query should
return values as either string or 0. This has been up for discussion
earlier, actually, and iirc the proposed names for those new functions
were typed_query and big_typed_query, respectively.

> +class SqlNULL
> +{
> +  constant is_SqlNULL = 1;
> +
> +  // Treat these objects as indistinguishable from each other. We
> +  // ought to ensure that there's only one in the pike process
> +  // instead, but that's tricky to solve in the PCode codec.
> +  protected int `== (mixed other) {
> +    return zero_type(other)
> +     || objectp(other) && other->is_SqlNULL;
> +  }

Thought I recognized that comment. ;) Actually, in a pike module that
uniqueness problem shouldn't exist, so the kludge isn't necessary.

> +  protected int(0..1) _is_type(mixed type) {
> +    return 1;
> +  }

I'm not sure this is a good idea. Is there a good palpable reason to
"lie" in type checks like this? Because if there isn't, then the
default, I think, is "don't".

> +  protected mixed cast(string to) {
> +    mixed ret=UNDEFINED;
> +    switch(to) {
> +      case "string":
> + ret="";
> + break;
> +      case "float":
> + ret=Math.nan;
> + break;
> +      case "mixed":case "object":
> + ret=this;
> +    }
> +    return ret;
> +  }

Casting to unsupported types ought to throw errors, not silently
return UNDEFINED.

> +  protected int __hash() {
> +    return 17;
> +  }

This is the second part of the kludge above. It can be removed too.

[Stephen R. van den Berg]

Martin Stjernholm,  Roxen IS @ Pike  developers forum wrote:
>> > Well, actually, a cursory check amongst database backends in the
>> current (and older) Pike versions shows:

>> - Mysql       string/float/int

>Your check appears to be a little too cursory; this case is string
>only (since OLD_SQL_COMPAT is typically not defined). Although it's

Hmmm, indeed.

>only one backend, it's in my world the by far most important one.

I understand that, that's why I listed it as the first one.
Incidentally, (my own) benchmarks show that for the single-threaded
simple queries (few tables) with less than 10 rows of dataresults, MySQL
outperforms PostgreSQL by a factor of two.  However, performance of MySQL
steadily drops for larger resultsets, more tables and multithreading;
whereas PostgreSQL's performance stays roughly the same.

>> - pgsql       string/float/int

>This is your new one, right? Then, I'm sorry to say, it has little
>relevance in this discussion.

I only included it for comparison, not to bring weight to the argument;
I already figured that the MySQL driver would be the most compelling
example.

What set me off on the wrong foot, probably, were comments in the
Postgres driver which *complained* about the fact that the postgres
libpq libs "only" return strings and not binary formats.
It seems that through the years, the Sql interface wanted to have real
types, but slowly mutated to strings only.

>An Sql.NULL value would be good, but this would also need new API
>calls, since I stubbornly maintain that query and big_query should
>return values as either string or 0. This has been up for discussion
>earlier, actually, and iirc the proposed names for those new functions
>were typed_query and big_typed_query, respectively.

I think I missed that discussion, but maybe it was held before I started
reading pike-devel.

>> +  // Treat these objects as indistinguishable from each other. We
>> +  // ought to ensure that there's only one in the pike process
>> +  // instead, but that's tricky to solve in the PCode codec.

>Thought I recognized that comment. ;) Actually, in a pike module that

Never create what you can steal ;-).

>uniqueness problem shouldn't exist, so the kludge isn't necessary.

>> +  protected int(0..1) _is_type(mixed type) {
>> +    return 1;
>> +  }

>I'm not sure this is a good idea. Is there a good palpable reason to
>"lie" in type checks like this? Because if there isn't, then the
>default, I think, is "don't".

Well, a NULL value basically has any type (in SQL).  I'm not sure what
would be the most natural way to extend this into Pike, but I figured
that the closest we could get, was something like the code above.
Since the NULL value has "any" type, it's not lying as such.

>> +  protected mixed cast(string to) {
>> +    mixed ret=UNDEFINED;
>> +    switch(to) {
>> +      case "string":
>> + ret="";
>> + break;
>> +      case "float":
>> + ret=Math.nan;
>> + break;
>> +      case "mixed":case "object":
>> + ret=this;
>> +    }
>> +    return ret;
>> +  }

>Casting to unsupported types ought to throw errors, not silently
>return UNDEFINED.

The problem is, the NULL value *can* be casted to any type, it's just
that the result for any types other than the ones mentioned above is
just that: undefined.
--
Sincerely,
           Stephen R. van den Berg.
Auto repair rates: basic labor $40/hour; if you wait, $60; if you watch, $80;
if you ask questions, $100; if you help, $120; if you laugh, $140.

> /.../ It seems that through the years, the Sql interface wanted to
> have real types, but slowly mutated to strings only.

Yes, that's suggested by the OLD_SQL_COMPAT define too. I'm not sure
how it came about, but it might have been an odd way to fix the
integer 0 vs. NULL problem.

> Well, a NULL value basically has any type (in SQL).  I'm not sure what
> would be the most natural way to extend this into Pike, but I figured
> that the closest we could get, was something like the code above.
> Since the NULL value has "any" type, it's not lying as such.

I think one should view this more from the Pike angle. If e.g.
arrayp(x) is true, a pike programmer typically expects that all
operators and calls for arrays would work for x. It's not possible to
achieve that when x is an object (most builtin functions expecting
arrays would still balk, for example). So even if you added a bunch of
backtick functions it'd still not work 100%.

That's why I think _is_type should be used very sparingly. In this
case we don't have compatibility concerns (assuming new query calls),
so there's no need for pretence for that reason.

Adding some backticks like `+ and ``+ to make it behave a little bit
like builtin types is another matter. That's just a convenience; the
user must still know that there's really an object involved.

> The problem is, the NULL value *can* be casted to any type, it's just
> that the result for any types other than the ones mentioned above is
> just that: undefined.

Here again I think it's more right to view it from a Pike perspective
rather than SQL, since it actually is Pike we got at this end of the
interface. The return value from e.g. cast("multiset") should really
be a multiset, and UNDEFINED isn't a multiset. If a cast function
cannot return a value of the indicated type, it should always throw an
error.

[Stephen R. van den Berg]

Martin Stjernholm,  Roxen IS @ Pike  developers forum wrote:
>> Well, a NULL value basically has any type (in SQL).  I'm not sure what
>> would be the most natural way to extend this into Pike, but I figured
>> that the closest we could get, was something like the code above.
>> Since the NULL value has "any" type, it's not lying as such.

>I think one should view this more from the Pike angle. If e.g.
>arrayp(x) is true, a pike programmer typically expects that all
>operators and calls for arrays would work for x. It's not possible to
>achieve that when x is an object (most builtin functions expecting
>arrays would still balk, for example). So even if you added a bunch of
>backtick functions it'd still not work 100%.

Ok, I see your point.  I'll try and pickup the slack as soon as 7.9 sees
the light and help to get this going.
--
Sincerely,
           Stephen R. van den Berg.

"Good moaning!"

>Ok, I see your point.  I'll try and pickup the slack as soon as 7.9 sees
>the light and help to get this going.

Specifically from the point of Sql it would be good to make a new
interface altogether, as has previously been discussed, basing it on
inheritance instead of proxy functions. If you (or anyone else for
that matter) could create a good looking API, half the work is done.

[Stephen R. van den Berg]

Martin Nilsson (Opera Mini - AFK!) @ Pike (-) developers forum wrote:
>>Ok, I see your point.  I'll try and pickup the slack as soon as 7.9 sees
>>the light and help to get this going.

>Specifically from the point of Sql it would be good to make a new
>interface altogether, as has previously been discussed, basing it on
>inheritance instead of proxy functions. If you (or anyone else for
>that matter) could create a good looking API, half the work is done.

That is part of the deal yes.  I'll look into that and propose
something.
--
Sincerely,
           Stephen R. van den Berg.

"Good moaning!"