[GRASS-dev] [GRASS GIS] #2851: v.in.ogr: imports empty values as empty strings instead of NULL values

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
-------------------------+-------------------------
Reporter: mlennert | Owner: grass-dev@…
     Type: defect | Status: new
Priority: normal | Milestone: 7.0.3
Component: Default | Version: unspecified
Keywords: | CPU: Unspecified
Platform: Unspecified |
-------------------------+-------------------------
v.in.ogr imports empty values in string fields (including date fields) as
empty strings and not as NULL values.

Is there any specific reason for that ?

If not, I've attached a patch that changes that. But before applying, I
would like to hear if anyone has an opinion on why this might or might not
be a good idea.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+-------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Default | Version: unspecified
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by mlennert):

* Attachment "v_in_ogr_null_handling.diff" added.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------
Changes (by mlennert):

* keywords: => vector v.in.ogr null
* version: unspecified => svn-trunk

Comment:

v.in.ogr imports empty values in string fields (including date fields) as
empty strings and not as NULL values.

Is there any specific reason for that ?

If not, I've attached a patch against trunk that changes that. But before
applying, I would like to hear if anyone has an opinion on why this might
or might not be a good idea.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:1&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------

Comment (by marisn):

Although I am more purist and would say that NULL should be NULL, here's
opinion why "no data" should be text (from Django framework):

"Avoid using null on string-based fields such as CharField and TextField
because empty string values will always be stored as empty strings, not as
NULL. If a string-based field has null=True, that means it has two
possible values for “no data”: NULL, and the empty string. In most cases,
it’s redundant to have two possible values for “no data;” the Django
convention is to use the empty string, not NULL."
https://docs.djangoproject.com/en/1.9/ref/models/fields/#null

If it comes down to voting, my vote goes for keeping NULL as NULL (do not
alter data unless asked for).

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:2&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------

Comment (by mlennert):

Replying to [comment:2 marisn]:
> Although I am more purist and would say that NULL should be NULL, here's
opinion why "no data" should be text (from Django framework):
>
> "Avoid using null on string-based fields such as CharField and TextField
because empty string values will always be stored as empty strings, not as
NULL. If a string-based field has null=True, that means it has two
possible values for “no data”: NULL, and the empty string. In most cases,
it’s redundant to have two possible values for “no data;” the Django
convention is to use the empty string, not NULL."
> https://docs.djangoproject.com/en/1.9/ref/models/fields/#null
>
> If it comes down to voting, my vote goes for keeping NULL as NULL (do
not alter data unless asked for).

I agree with you. NULL is a concept that is different from the empty
string, even though in some cases they are equivalent (e.g. AFAIK there is
no way to distinguish between the two in a csv file...).

In the case of this specific bug the issue is specifically for date fields
which, IIUC, are only supported as such in OGR since 1.3 (but even Debian
Squeeze, aka oldoldstable is beyond that version, so that should not be an
issue). So a compromise solution could be to nullify the date and time
fields (i.e. the first change in the patch), but not the string fields.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:3&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.3
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------

Comment (by mlennert):

I've committed the change for both date and string types in trunk in
r67516. That way this will get some testing. Not sure if this should
already be backported to release70 or if we should wait for some more
testing.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:4&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.4
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------

Comment (by neteler):

Shall r67516 be backported?

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:7&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: new
  Priority: normal | Milestone: 7.0.4
Component: Default | Version: svn-trunk
Resolution: | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------

Comment (by mlennert):

Replying to [comment:7 neteler]:
> Shall r67516 be backported?

I just saw on the website that you officially released 7.0.3, so yes, I'll
backport.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:8&gt;
GRASS GIS <https://grass.osgeo.org>

#2851: v.in.ogr: imports empty values as empty strings instead of NULL values
--------------------------+----------------------------------
  Reporter: mlennert | Owner: grass-dev@…
      Type: defect | Status: closed
  Priority: normal | Milestone: 7.0.4
Component: Default | Version: svn-trunk
Resolution: fixed | Keywords: vector v.in.ogr null
       CPU: Unspecified | Platform: Unspecified
--------------------------+----------------------------------
Changes (by mlennert):

* status: new => closed
* resolution: => fixed

Comment:

Replying to [comment:8 mlennert]:
> Replying to [comment:7 neteler]:
> > Shall r67516 be backported?
>
> I just saw on the website that you officially released 7.0.3, so yes,
I'll backport.

Done (r67702).

Closing this bug for now.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/2851#comment:9&gt;
GRASS GIS <https://grass.osgeo.org>