-
-
Notifications
You must be signed in to change notification settings - Fork 307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v.in.ogr: skip columns with unsupported data type instead of failing to import #2630
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just had some minor remarks.
Instead of adding a special case for |
vector/v.in.ogr/main.c
Outdated
Ogr_field = OGR_FD_GetFieldDefn(Ogr_featuredefn, i); | ||
Ogr_ftype = OGR_Fld_GetType(Ogr_field); | ||
|
||
G_debug(3, "Ogr_ftype: %i", Ogr_ftype); /* look up below */ | ||
|
||
/* skip columns with unsupported data type */ | ||
if (Ogr_ftype == OFTBinary) { | ||
G_warning(_("Column <%s> is omitted, binary data type is not supported."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supported data types are handled below at L1184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! And there all columns with unsupported data types may be omitted. However, that would be a break with present behaviour (keep column, but omit rows). Perhaps a new flag for “omit columns with unsupported data types”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but isn't the binary column then included in the output table creation struct grass_col_info
already in L1091:
Line 1091 in 5d64de2
col_info[i_out].idx = i_out; |
So, I guess if we fix it in L1184, it would have to be removed from col_info
?
How would I do that? A web-search unfortunately did not give me a straight forward solution...
There is a deeper bug in |
G_warning(_("Column type (Ogr_ftype: %d) not supported (Ogr_fieldname: %s)"), | ||
Ogr_ftype, Ogr_fieldname); | ||
buf[0] = 0; | ||
col_info[i_out].type = G_store(buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the current behaviour, add
i_out--;
remove
col_info[i_out].type = G_store(buf);
vector/v.in.ogr/main.c
Outdated
/* handle columns of unsupported data type */ | ||
G_warning(_("Column type (Ogr_ftype: %d) not supported (Ogr_fieldname: %s)\n"), | ||
Ogr_ftype, Ogr_fieldname); | ||
if (flag.drop->answer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, the drop flag does not make sense: unsupported column types can not be transferred in any reasonable way.
When filling attributes, the code block at needs to be removed:
this is the hidden bug: should be |
Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com>
Thanks again. Suggestions implemented in 5cf030a. I hope I got those right... |
P.S.: I modified the warning message a bit in an attempt to make it a bit more clear what is happening. Happy to change it back if preferred... |
With latest changes, may I suggest the following warning message: G_warning(_("Column <%s> is of unsupported \"%s\" data type, "
"it is omitted from import."),
Ogr_fieldname, OGR_GetFieldTypeName(Ogr_ftype)); which will result in a message like:
General note: G_warning will automatically reflow the printed message to fit in a 80 col terminal, no need to break |
@ninsbl Can you please update the PR description to reflect the current intention and changes done? (will be needed for the merge/final commit anyway) |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good! I'm wondering if the line is not too long, but formatting in this file needs fixing anyway, so maybe not a big deal as long as we are planning to fix it soon.
…to import (#2630) * skip binary columns * remove copied comment * indent + warning message * handle all unsupported column types with d-flag * remove second warning * Update vector/v.in.ogr/main.c Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com> * implement suggestion from review * cleanup from review * simplify warning Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com>
…to import (OSGeo#2630) * skip binary columns * remove copied comment * indent + warning message * handle all unsupported column types with d-flag * remove second warning * Update vector/v.in.ogr/main.c Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com> * implement suggestion from review * cleanup from review * simplify warning Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com>
…to import (OSGeo#2630) * skip binary columns * remove copied comment * indent + warning message * handle all unsupported column types with d-flag * remove second warning * Update vector/v.in.ogr/main.c Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com> * implement suggestion from review * cleanup from review * simplify warning Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com>
…to import (OSGeo#2630) * skip binary columns * remove copied comment * indent + warning message * handle all unsupported column types with d-flag * remove second warning * Update vector/v.in.ogr/main.c Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com> * implement suggestion from review * cleanup from review * simplify warning Co-authored-by: Markus Metz <33666869+metzm@users.noreply.github.com>
Before this PR, vector data failed to import if it contained columns in the attribute table that are of a data type that is not supported by GRASS` s DBMI - like binary data (BLOB). This PR changes the behavior of
v.in.ogr
to drop such columns from import instead of failing to import the data in total.