Opened 3 years ago

Closed 3 years ago

#2851 closed defect (fixed)

v.in.ogr: imports empty values as empty strings instead of NULL values

Reported by: mlennert Owned by: grass-dev@…
Priority: normal Milestone: 7.0.4
Component: Default Version: svn-trunk
Keywords: vector v.in.ogr null Cc:
CPU: Unspecified Platform: Unspecified

Description

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.

Attachments (1)

v_in_ogr_null_handling.diff (682 bytes) - added by mlennert 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by mlennert

Attachment: v_in_ogr_null_handling.diff added

comment:1 Changed 3 years ago by mlennert

Keywords: vector v.in.ogr null added
Version: unspecifiedsvn-trunk

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.

comment:2 Changed 3 years ago 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).

comment:3 in reply to:  2 Changed 3 years ago by mlennert

Replying to 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.

comment:4 Changed 3 years ago 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.

comment:5 Changed 3 years ago by neteler

Milestone: 7.0.3

Ticket retargeted after milestone closed

comment:6 Changed 3 years ago by neteler

Milestone: 7.0.4

Ticket retargeted after 7.0.3 milestone closed

comment:7 Changed 3 years ago by neteler

Shall r67516 be backported?

comment:8 in reply to:  7 ; Changed 3 years ago by mlennert

Replying to neteler:

Shall r67516 be backported?

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

comment:9 in reply to:  8 Changed 3 years ago by mlennert

Resolution: fixed
Status: newclosed

Replying to mlennert:

Replying to 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.

Note: See TracTickets for help on using tickets.