From 042fab20de1687aded1905864bec0ebaea1614e7 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Mon, 30 Aug 2021 20:04:23 -0700 Subject: [PATCH 01/14] Return `missing` if the field is set but null. Fixes #177 . I think this aligns the behavior a lot better across the different file formats (see the test sets in test/test_tables.jl) This behavior is implemented through `getfield(feature, i)` which makes use of `getfieldtype(feature, i)`. That way, we are aligned in behavior for field subtypes even for e.g. displaying the fields. Because a lot hinges on distinguishing between whether a field is set versus whether it is null (see https://github.com/Toblerity/Fiona/issues/460#issue-240013079), I have also added support for `isfieldnull()`, `isfieldsetandnotnull()`, and `setfieldnull!()`. --- src/base/display.jl | 2 +- src/ogr/feature.jl | 54 +++++++++++++++++++++++++++++--- src/ogr/fielddefn.jl | 20 ++++++++++++ test/test_feature.jl | 15 +++++++++ test/test_fielddefn.jl | 5 +++ test/test_tables.jl | 71 ++++++++++++++++++------------------------ 6 files changed, 122 insertions(+), 45 deletions(-) diff --git a/src/base/display.jl b/src/base/display.jl index 64b6e435..3f7cbe11 100644 --- a/src/base/display.jl +++ b/src/base/display.jl @@ -149,7 +149,7 @@ function Base.show(io::IO, layer::AbstractFeatureLayer)::Nothing nfielddisplay = min(n, 5) for i in 1:nfielddisplay fd = getfielddefn(featuredefn, i - 1) - display = " Field $(i - 1) ($(getname(fd))): [$(gettype(fd))]" + display = " Field $(i - 1) ($(getname(fd))): [$(getfieldtype(fd))]" if length(display) > 75 println(io, "$display[1:70]...") continue diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 348af72f..241adfe0 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -149,6 +149,50 @@ function unsetfield!(feature::Feature, i::Integer)::Feature return feature end +""" + isfieldnull(feature::Feature, i::Integer) + +Test if a field is null. + +### Parameters +* `feature`: the feature that owned the field. +* `i`: the field to test, from 0 to GetFieldCount()-1. + +### Returns +`true` if the field is null, otherwise `false`. +""" +isfieldnull(feature::Feature, i::Integer)::Bool = + Bool(GDAL.ogr_f_isfieldnull(feature.ptr, i)) + +""" + isfieldsetandnotnull(feature::Feature, i::Integer) + +Test if a field is set and not null. + +### Parameters +* `feature`: the feature that owned the field. +* `i`: the field to test, from 0 to GetFieldCount()-1. + +### Returns +`true` if the field is null, otherwise `false`. +""" +isfieldsetandnotnull(feature::Feature, i::Integer)::Bool = + Bool(GDAL.ogr_f_isfieldsetandnotnull(feature.ptr, i)) + +""" + setfieldnull!(feature::Feature, i::Integer) + +Clear a field, marking it as null. + +### Parameters +* `feature`: the feature that owned the field. +* `i`: the field to set to null, from 0 to GetFieldCount()-1. +""" +function setfieldnull!(feature::Feature, i::Integer)::Feature + GDAL.ogr_f_setfieldnull(feature.ptr, i) + return feature +end + # """ # OGR_F_GetRawFieldRef(OGRFeatureH hFeat, # int iField) -> OGRField * @@ -403,12 +447,14 @@ const _FETCHFIELD = Dict{OGRFieldType,Function}( ) function getfield(feature::Feature, i::Integer) - return if isfieldset(feature, i) - _fieldtype = gettype(getfielddefn(feature, i)) + return if !isfieldset(feature, i) + getdefault(feature, i) + elseif isfieldnull(feature, i) + missing + else + _fieldtype = getfieldtype(getfielddefn(feature, i)) _fetchfield = get(_FETCHFIELD, _fieldtype, getdefault) _fetchfield(feature, i) - else - getdefault(feature, i) end end diff --git a/src/ogr/fielddefn.jl b/src/ogr/fielddefn.jl index ec183f55..bf742cf7 100644 --- a/src/ogr/fielddefn.jl +++ b/src/ogr/fielddefn.jl @@ -71,6 +71,26 @@ function setsubtype!(fielddefn::FieldDefn, subtype::OGRFieldSubType)::FieldDefn return fielddefn end +""" + getfieldtype(fielddefn::AbstractFieldDefn) + +Returns the type or subtype (if any) of this field. + +### Parameters +* `fielddefn`: handle to the field definition. + +### Returns +The field type or subtype. +""" +function getfieldtype(fielddefn::AbstractFieldDefn)::Union{OGRFieldType, OGRFieldSubType} + fieldsubtype = getsubtype(fielddefn) + return if fieldsubtype != OFSTNone + fieldsubtype + else + gettype(fielddefn) + end +end + """ getjustify(fielddefn::AbstractFieldDefn) diff --git a/test/test_feature.jl b/test/test_feature.jl index 3f7a31b9..db1d7cc8 100644 --- a/test/test_feature.jl +++ b/test/test_feature.jl @@ -157,6 +157,10 @@ const AG = ArchGDAL; AG.setfield!(feature, 9, Int16(1)) AG.setfield!(feature, 10, Int32(1)) AG.setfield!(feature, 11, Float32(1.0)) + for i in 1:AG.nfield(feature) + @test !AG.isfieldnull(feature, i-1) + @test AG.isfieldsetandnotnull(feature, i-1) + end @test sprint(print, AG.getgeom(feature)) == "NULL Geometry" AG.getgeom(feature) do geom @test sprint(print, geom) == "NULL Geometry" @@ -169,6 +173,10 @@ const AG = ArchGDAL; AG.addfeature(layer) do newfeature AG.setfrom!(newfeature, feature) @test AG.getfield(newfeature, 0) == 1 + for i in 1:AG.nfield(newfeature) + @test !AG.isfieldnull(newfeature, i-1) + @test AG.isfieldsetandnotnull(newfeature, i-1) + end @test AG.getfield(newfeature, 1) ≈ 1.0 @test AG.getfield(newfeature, 2) == Int32[1, 2] @test AG.getfield(newfeature, 3) == Int64[1, 2] @@ -209,6 +217,13 @@ const AG = ArchGDAL; @test AG.getfield(newfeature, 0) == 45 @test AG.getfield(newfeature, 1) ≈ 18.2 @test AG.getfield(newfeature, 5) == String["foo", "bar"] + + @test AG.isfieldsetandnotnull(newfeature, 5) + AG.setfieldnull!(newfeature, 5) + @test !AG.isfieldsetandnotnull(newfeature, 5) + @test AG.isfieldset(newfeature, 5) + @test AG.isfieldnull(newfeature, 5) + @test ismissing(AG.getfield(newfeature, 5)) end @test AG.nfeature(layer) == 1 end diff --git a/test/test_fielddefn.jl b/test/test_fielddefn.jl index 88fa2688..8a97e3a8 100644 --- a/test/test_fielddefn.jl +++ b/test/test_fielddefn.jl @@ -10,14 +10,19 @@ const AG = ArchGDAL; AG.setname!(fd, "newname") @test AG.getname(fd) == "newname" @test AG.gettype(fd) == AG.OFTInteger + @test AG.getfieldtype(fd) == AG.OFTInteger AG.settype!(fd, AG.OFTDate) @test AG.gettype(fd) == AG.OFTDate + @test AG.getfieldtype(fd) == AG.OFTDate AG.settype!(fd, AG.OFTInteger) @test AG.getsubtype(fd) == AG.OFSTNone + @test AG.getfieldtype(fd) == ArchGDAL.OFTInteger AG.setsubtype!(fd, AG.OFSTInt16) @test AG.getsubtype(fd) == AG.OFSTInt16 + @test AG.getfieldtype(fd) == AG.OFSTInt16 AG.setsubtype!(fd, AG.OFSTBoolean) @test AG.getsubtype(fd) == AG.OFSTBoolean + @test AG.getfieldtype(fd) == AG.OFSTBoolean AG.setsubtype!(fd, AG.OFSTNone) @test AG.getjustify(fd) == AG.OJUndefined AG.setjustify!(fd, AG.OJLeft) diff --git a/test/test_tables.jl b/test/test_tables.jl index aa79f948..bcb5682b 100644 --- a/test/test_tables.jl +++ b/test/test_tables.jl @@ -488,7 +488,7 @@ using Tables withmissingfield::Bool, withmixedgeomtypes::Bool, reference_geotable::NamedTuple, - )::Bool + ) map_on_test_dataset( drvshortname, geomfamilly; @@ -497,18 +497,14 @@ using Tables withmixedgeomtypes = withmixedgeomtypes, ) do ds layer = AG.getlayer(ds, 0) - return all([ - keys(Tables.columntable(layer)) == - reference_geotable.names, - eltype.(values(Tables.columntable(layer))) == - reference_geotable.types, - tupleoftuples_equal( - columntablevalues_toWKT( - values(Tables.columntable(layer)), - ), - reference_geotable.values, + @test keys(Tables.columntable(layer)) == reference_geotable.names + @test eltype.(values(Tables.columntable(layer))) == reference_geotable.types + @test tupleoftuples_equal( + columntablevalues_toWKT( + values(Tables.columntable(layer)), ), - ]) + reference_geotable.values, + ) end end @@ -524,24 +520,21 @@ using Tables function test_layer_to_table( layer::AG.AbstractFeatureLayer, reference_geotable::NamedTuple, - )::Bool - return all([ - keys(Tables.columntable(layer)) == reference_geotable.names, - eltype.(values(Tables.columntable(layer))) == - reference_geotable.types, - tupleoftuples_equal( - columntablevalues_toWKT( - values(Tables.columntable(layer)), - ), - reference_geotable.values, + ) + @test keys(Tables.columntable(layer)) == reference_geotable.names + @test eltype.(values(Tables.columntable(layer))) == reference_geotable.types + @test tupleoftuples_equal( + columntablevalues_toWKT( + values(Tables.columntable(layer)), ), - ]) + reference_geotable.values, + ) end @testset "Conversion to table for ESRI Shapefile driver" begin ESRI_Shapefile_test_reference_geotable = ( names = (Symbol(""), :id, :name), - types = (Union{Missing,ArchGDAL.IGeometry}, Int64, String), + types = (Union{Missing,ArchGDAL.IGeometry}, Union{Missing,Int64}, String), values = ( Union{Missing,String}[ "POLYGON ((0 0,0 1,1 1))", @@ -549,11 +542,11 @@ using Tables missing, "POLYGON ((0 0,-1 0,-1 1))", ], - [1, 2, 3, 0], + [1, 2, 3, missing], ["polygon1", "multipolygon1", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "ESRI Shapefile", "polygon", true, @@ -569,7 +562,7 @@ using Tables Missing, ArchGDAL.IGeometry{ArchGDAL.wkbLineString}, }, - Int64, + Union{Missing,Int64}, String, ), values = ( @@ -579,11 +572,11 @@ using Tables missing, "LINESTRING (5 6,6 7,7 8)", ], - [1, 2, 3, 0], + [1, 2, 3, missing], ["line1", "line2", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "ESRI Shapefile", "line", true, @@ -612,7 +605,7 @@ using Tables ["polygon1", "multipolygon1", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "GeoJSON", "polygon", true, @@ -642,7 +635,7 @@ using Tables ["line1", "line2", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "GeoJSON", "line", true, @@ -678,7 +671,7 @@ using Tables ["line1", "multiline1", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "GML", "line", true, @@ -691,7 +684,7 @@ using Tables @testset "Conversion to table for GPKG driver" begin GPKG_test_reference_geotable = ( names = (:geom, :id, :name), - types = (Union{Missing,ArchGDAL.IGeometry}, Int64, String), + types = (Union{Missing,ArchGDAL.IGeometry}, Union{Missing,Int64}, String), values = ( Union{Missing,String}[ "LINESTRING (1 2,2 3,3 4)", @@ -699,11 +692,11 @@ using Tables missing, "LINESTRING (5 6,6 7,7 8)", ], - [1, 2, 3, 0], + Union{Missing,Int64}[1, 2, 3, missing], ["line1", "multiline1", "emptygeom", "emptyid"], ), ) - @test test_layer_to_table( + test_layer_to_table( "GPKG", "line", true, @@ -727,7 +720,7 @@ using Tables ["", "", ""], ), ) - @test test_layer_to_table( + test_layer_to_table( "KML", "line", true, @@ -751,7 +744,7 @@ using Tables ["emptyid", "multiline1", "line1"], ), ) - @test test_layer_to_table( + test_layer_to_table( "FlatGeobuf", "line", true, @@ -762,7 +755,6 @@ using Tables end @testset "Conversion to table for CSV driver" begin - @test begin AG.read( joinpath(@__DIR__, "data/multi_geom.csv"), options = [ @@ -797,11 +789,10 @@ using Tables ["Mumbai", "New Delhi"], ), ) - return test_layer_to_table( + test_layer_to_table( multigeom_test_layer, CSV_multigeom_test_reference_geotable, ) - end end end From e59dd31af5286b54160cea3df0b530af78bad8ae Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Tue, 31 Aug 2021 18:41:20 -0700 Subject: [PATCH 02/14] Add docstring references for GDAL/OGR RFCs --- src/ogr/feature.jl | 21 +++++++++++++++++++++ src/ogr/fielddefn.jl | 21 +++++++++++++++++++++ src/types.jl | 6 ++++++ 3 files changed, 48 insertions(+) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 241adfe0..65e92e60 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -160,6 +160,9 @@ Test if a field is null. ### Returns `true` if the field is null, otherwise `false`. + +### References +* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html """ isfieldnull(feature::Feature, i::Integer)::Bool = Bool(GDAL.ogr_f_isfieldnull(feature.ptr, i)) @@ -175,6 +178,9 @@ Test if a field is set and not null. ### Returns `true` if the field is null, otherwise `false`. + +### References +* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html """ isfieldsetandnotnull(feature::Feature, i::Integer)::Bool = Bool(GDAL.ogr_f_isfieldsetandnotnull(feature.ptr, i)) @@ -187,6 +193,9 @@ Clear a field, marking it as null. ### Parameters * `feature`: the feature that owned the field. * `i`: the field to set to null, from 0 to GetFieldCount()-1. + +### References +* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html """ function setfieldnull!(feature::Feature, i::Integer)::Feature GDAL.ogr_f_setfieldnull(feature.ptr, i) @@ -446,6 +455,12 @@ const _FETCHFIELD = Dict{OGRFieldType,Function}( OFTInteger64List => asint64list, ) +""" + getfield(feature, i) + +### References +* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html +""" function getfield(feature::Feature, i::Integer) return if !isfieldset(feature, i) getdefault(feature, i) @@ -900,6 +915,9 @@ Fill unset fields with default values that might be defined. * `feature`: handle to the feature. * `notnull`: if we should fill only unset fields with a not-null constraint. * `papszOptions`: unused currently. Must be set to `NULL`. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ function fillunsetwithdefault!( feature::Feature; @@ -931,6 +949,9 @@ fails, then it will fail for all interpretations). ### Returns `true` if all enabled validation tests pass. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ validate(feature::Feature, flags::FieldValidation, emiterror::Bool)::Bool = Bool(GDAL.ogr_f_validate(feature.ptr, flags, emiterror)) diff --git a/src/ogr/fielddefn.jl b/src/ogr/fielddefn.jl index bf742cf7..3a587869 100644 --- a/src/ogr/fielddefn.jl +++ b/src/ogr/fielddefn.jl @@ -65,6 +65,9 @@ OGRFeatureDefn. ### Parameters * `fielddefn`: handle to the field definition to set type to. * `subtype`: the new field subtype. + +### References +* https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html """ function setsubtype!(fielddefn::FieldDefn, subtype::OGRFieldSubType)::FieldDefn GDAL.ogr_fld_setsubtype(fielddefn.ptr, subtype) @@ -81,6 +84,9 @@ Returns the type or subtype (if any) of this field. ### Returns The field type or subtype. + +### References +* https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html """ function getfieldtype(fielddefn::AbstractFieldDefn)::Union{OGRFieldType, OGRFieldSubType} fieldsubtype = getsubtype(fielddefn) @@ -216,6 +222,9 @@ Even if this method returns `false` (i.e not-nullable field), it doesn't mean that OGRFeature::IsFieldSet() will necessarily return `true`, as fields can be temporarily unset and null/not-null validation is usually done when OGRLayer::CreateFeature()/SetFeature() is called. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ isnullable(fielddefn::AbstractFieldDefn)::Bool = Bool(GDAL.ogr_fld_isnullable(fielddefn.ptr)) @@ -230,6 +239,9 @@ to set a not-null constraint. Drivers that support writing not-null constraint will advertize the GDAL_DCAP_NOTNULL_FIELDS driver metadata item. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ function setnullable!(fielddefn::FieldDefn, nullable::Bool)::FieldDefn GDAL.ogr_fld_setnullable(fielddefn.ptr, nullable) @@ -240,6 +252,9 @@ end getdefault(fielddefn::AbstractFieldDefn) Get default field value + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ function getdefault(fielddefn::AbstractFieldDefn)::Union{String,Missing} result = @@ -272,6 +287,9 @@ datetime literal value, format should be 'YYYY/MM/DD HH:MM:SS[.sss]' Drivers that support writing DEFAULT clauses will advertize the GDAL_DCAP_DEFAULT_FIELDS driver metadata item. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ function setdefault!(fielddefn::T, default)::T where {T<:AbstractFieldDefn} GDAL.ogr_fld_setdefault(fielddefn.ptr, default) @@ -286,6 +304,9 @@ Returns whether the default value is driver specific. Driver specific default values are those that are not NULL, a numeric value, a literal value enclosed between single quote characters, CURRENT_TIMESTAMP, CURRENT_TIME, CURRENT_DATE or datetime literal value. + +### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ isdefaultdriverspecific(fielddefn::AbstractFieldDefn)::Bool = Bool(GDAL.ogr_fld_isdefaultdriverspecific(fielddefn.ptr)) diff --git a/src/types.jl b/src/types.jl index 4ef8bdd9..a2936a2b 100644 --- a/src/types.jl +++ b/src/types.jl @@ -619,6 +619,9 @@ getname(obj::OGRFieldType)::String = GDAL.ogr_getfieldtypename(obj) getname(obj::OGRFieldSubType) Fetch human readable name for a field subtype. + +### References +* https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html """ getname(obj::OGRFieldSubType)::String = GDAL.ogr_getfieldsubtypename(obj) @@ -626,6 +629,9 @@ getname(obj::OGRFieldSubType)::String = GDAL.ogr_getfieldsubtypename(obj) arecompatible(dtype::OGRFieldType, subtype::OGRFieldSubType) Return if type and subtype are compatible. + +### References +* https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html """ arecompatible(dtype::OGRFieldType, subtype::OGRFieldSubType)::Bool = Bool(GDAL.ogr_aretypesubtypecompatible(dtype, subtype)) From f18c8c6bb28e64c8543dae62e5e529ae6b50226e Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 21:43:52 -0700 Subject: [PATCH 03/14] Document the intended ArchGDAL behavior for NULL semantics --- docs/src/considerations.md | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/docs/src/considerations.md b/docs/src/considerations.md index 8b9ab4e0..77329e8b 100644 --- a/docs/src/considerations.md +++ b/docs/src/considerations.md @@ -96,3 +96,37 @@ The interface is implemented in [`src/tables.jl`](https://github.com/yeesian/Arc * `ArchGDAL.FeatureLayer` meets the criteria for an `AbstractRow`-iterator based on the previous bullet and meeting the criteria for [`Iteration`](https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-iteration) in [`base/iterators.jl`](https://github.com/yeesian/ArchGDAL.jl/blob/a665f3407930b8221269f8949c246db022c3a85c/src/base/iterators.jl#L1-L18). * `ArchGDAL.AbstractDataset` might contain multiple layers, and might correspond to multiple tables. The way to construct tables would be to get the layers before forming the corresponding tables. +## Missing and Null Semantics in GDAL + +When reading the fields of a feature, ArchGDAL observes the following behavior: + +| Field | null | notnull | +|-------|---------|---------| +| set | missing | value | +| unset | nothing | missing | + +This reflects that +* a field that is notnull will never return `nothing`. + * use `isfieldnull(feature, i)` to determine if a field has been set. +* a field that is set will never return `nothing`. + * use `isfieldset(feature, i)` to determine if a field has been set. +* a field that is set and not null will always have a concrete value. + * use `isfieldsetandnotnull(feature, i)` to test for it. + +When writing the fields of a feature, ArchGDAL observes the following behavior: + +| Field | nullable | notnullable | +|---------|----------|--------------| +| nothing | unset | unset | +| missing | null | getdefault() | +| value | value | value | + +This reflects that +* writing `nothing` will cause the field to be unset. +* writing `missing` will cause the field to be null. In the cause of a notnullable field, it will take the default value (see https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html for details). +* writing a value will behave in the usual manner. + +For additional references, see +* https://docs.julialang.org/en/v1/manual/faq/#faq-nothing +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html +* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html From bc2f0385f148fa06eeab2bf29aef42b42efd1988 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 22:50:59 -0700 Subject: [PATCH 04/14] Add tests and implementation for getfield with missing values As mathieu has observed, `OGRUnsetMarker` and `OGRNullMarkerare` are mutually exclusive. We implement that case to return nothing if it ever comes up, but it is not possible for us to the corresponding code path. --- src/ogr/fielddefn.jl | 2 +- test/test_feature.jl | 53 ++++++++++++++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/ogr/fielddefn.jl b/src/ogr/fielddefn.jl index 3a587869..33f513b7 100644 --- a/src/ogr/fielddefn.jl +++ b/src/ogr/fielddefn.jl @@ -243,7 +243,7 @@ GDAL_DCAP_NOTNULL_FIELDS driver metadata item. ### References * https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ -function setnullable!(fielddefn::FieldDefn, nullable::Bool)::FieldDefn +function setnullable!(fielddefn::T, nullable::Bool)::T where {T <: AbstractFieldDefn} GDAL.ogr_fld_setnullable(fielddefn.ptr, nullable) return fielddefn end diff --git a/test/test_feature.jl b/test/test_feature.jl index db1d7cc8..95aeff52 100644 --- a/test/test_feature.jl +++ b/test/test_feature.jl @@ -90,15 +90,50 @@ const AG = ArchGDAL; @test AG.validate(f, AG.F_VAL_ALLOW_DIFFERENT_GEOM_DIM, false) == true - @test AG.getfield(f, 1) == "point-a" - @test ismissing(AG.getdefault(f, 1)) - AG.setdefault!(AG.getfielddefn(f, 1), "default value") - @test AG.getdefault(f, 1) == "default value" - @test AG.getfield(f, 1) == "point-a" - AG.unsetfield!(f, 1) - @test AG.getfield(f, 1) == "default value" - AG.fillunsetwithdefault!(f, notnull = false) - @test AG.getfield(f, 1) == AG.getdefault(f, 1) + @testset "Missing and Null Semantics" begin + @test ismissing(AG.getdefault(f, 1)) + AG.setdefault!(AG.getfielddefn(f, 1), "default value") + @test AG.getdefault(f, 1) == "default value" + + @test AG.isfieldsetandnotnull(f, 1) + @test AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + @test AG.getfield(f, 1) == "point-a" + + AG.unsetfield!(f, 1) + @test !AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) # carried over from earlier + @test ismissing(AG.getfield(f, 1)) + + # unset & notnull: missing + AG.fillunsetwithdefault!(f) + @test ismissing(AG.getfield(f, 1)) # nothing has changed + @test AG.isnullable(AG.getfielddefn(f, 1)) # because it is a nullable field + @test !AG.isfieldnull(f, 1) # even though it is not a null value. + @test !AG.isfieldset(f, 1) # the field is still not set. + + # set & notnull: value + AG.fillunsetwithdefault!(f, notnull = false) # to set nullable fields + @test AG.getfield(f, 1) == AG.getdefault(f, 1) # now the field is set to the default + @test !AG.isfieldnull(f, 1) # still as expected + @test AG.isfieldset(f, 1) # the field is now set + + AG.setnullable!(AG.getfielddefn(f, 1), false) # set the field to be notnull. + AG.unsetfield!(f, 1) # now if we unset the field + @test !AG.isfieldnull(f, 1) && !AG.isfieldset(f, 1) && ismissing(AG.getfield(f, 1)) # to doublecheck + AG.fillunsetwithdefault!(f) # and we fill unset with default again + @test AG.getfield(f, 1) == AG.getdefault(f, 1) # the field is set to the default + + # set & null: missing + @test !AG.isfieldnull(f, 1) && AG.isfieldset(f, 1) # still as expected from before + AG.setfieldnull!(f, 1) + @test AG.isfieldnull(f, 1) && AG.isfieldset(f, 1) && ismissing(AG.getfield(f, 1)) + + # unset & null: N/A (but nothing otherwise) + AG.unsetfield!(f, 1) + # Observe that OGRUnsetMarker and OGRNullMarkerare are mutually exclusive + @test !AG.isfieldset(f, 1) && !AG.isfieldnull(f, 1) # notice the field is notnull + end end end From 958496206e31085aba53cb3ce5cb83aa09160a73 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 22:51:19 -0700 Subject: [PATCH 05/14] Update the implementation --- src/ogr/feature.jl | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 65e92e60..2cf3037a 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -459,17 +459,24 @@ const _FETCHFIELD = Dict{OGRFieldType,Function}( getfield(feature, i) ### References +* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html * https://gdal.org/development/rfc/rfc67_nullfieldvalues.html """ function getfield(feature::Feature, i::Integer) - return if !isfieldset(feature, i) - getdefault(feature, i) - elseif isfieldnull(feature, i) - missing - else + return if isfieldsetandnotnull(feature, i) + @assert isfieldset(feature, i) && !isfieldnull(feature, i) _fieldtype = getfieldtype(getfielddefn(feature, i)) _fetchfield = get(_FETCHFIELD, _fieldtype, getdefault) _fetchfield(feature, i) + elseif isfieldset(feature, i) + @assert isfieldnull(feature, i) + missing + elseif isfieldnull(feature, i) + @assert !isfieldset(feature, i) + nothing + else + @assert !isfieldset(feature, i) && !isfieldnull(feature, i) + missing end end From efd22c76d95fb723899df8bdce326bdb95d53614 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 23:21:43 -0700 Subject: [PATCH 06/14] Implement setting fields to missing or nothing. --- src/ogr/feature.jl | 14 ++++++++ test/test_feature.jl | 76 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 2cf3037a..135692ef 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -507,6 +507,20 @@ field types may be unaffected. """ function setfield! end +function setfield!(feature::Feature, i::Integer, value::Nothing)::Feature + unsetfield!(feature, i) + return feature +end + +function setfield!(feature::Feature, i::Integer, value::Missing)::Feature + if isnullable(getfielddefn(feature, i)) + setfieldnull!(feature, 1) + else + setfield!(feature, i, getdefault(feature, i)) + end + return feature +end + function setfield!(feature::Feature, i::Integer, value::Int32)::Feature GDAL.ogr_f_setfieldinteger(feature.ptr, i, value) return feature diff --git a/test/test_feature.jl b/test/test_feature.jl index 95aeff52..d165d4fe 100644 --- a/test/test_feature.jl +++ b/test/test_feature.jl @@ -107,32 +107,78 @@ const AG = ArchGDAL; # unset & notnull: missing AG.fillunsetwithdefault!(f) - @test ismissing(AG.getfield(f, 1)) # nothing has changed - @test AG.isnullable(AG.getfielddefn(f, 1)) # because it is a nullable field - @test !AG.isfieldnull(f, 1) # even though it is not a null value. - @test !AG.isfieldset(f, 1) # the field is still not set. + # nothing has changed + @test ismissing(AG.getfield(f, 1)) + # because it is a nullable field + @test AG.isnullable(AG.getfielddefn(f, 1)) + # even though it is not a null value + @test !AG.isfieldnull(f, 1) + # the field is still not set + @test !AG.isfieldset(f, 1) # set & notnull: value - AG.fillunsetwithdefault!(f, notnull = false) # to set nullable fields - @test AG.getfield(f, 1) == AG.getdefault(f, 1) # now the field is set to the default + AG.fillunsetwithdefault!(f, notnull = false) + # now the field is set to the default + @test AG.getfield(f, 1) == AG.getdefault(f, 1) @test !AG.isfieldnull(f, 1) # still as expected @test AG.isfieldset(f, 1) # the field is now set - AG.setnullable!(AG.getfielddefn(f, 1), false) # set the field to be notnull. - AG.unsetfield!(f, 1) # now if we unset the field - @test !AG.isfieldnull(f, 1) && !AG.isfieldset(f, 1) && ismissing(AG.getfield(f, 1)) # to doublecheck - AG.fillunsetwithdefault!(f) # and we fill unset with default again - @test AG.getfield(f, 1) == AG.getdefault(f, 1) # the field is set to the default + # set the field to be notnullable + AG.setnullable!(AG.getfielddefn(f, 1), false) + # now if we unset the field + AG.unsetfield!(f, 1) + @test !AG.isfieldnull(f, 1) + @test !AG.isfieldset(f, 1) + @test ismissing(AG.getfield(f, 1)) + # and we fill unset with default again + AG.fillunsetwithdefault!(f) + # the field is set to the default + @test AG.getfield(f, 1) == AG.getdefault(f, 1) # set & null: missing - @test !AG.isfieldnull(f, 1) && AG.isfieldset(f, 1) # still as expected from before + @test !AG.isfieldnull(f, 1) + @test AG.isfieldset(f, 1) AG.setfieldnull!(f, 1) - @test AG.isfieldnull(f, 1) && AG.isfieldset(f, 1) && ismissing(AG.getfield(f, 1)) + @test AG.isfieldnull(f, 1) + @test AG.isfieldset(f, 1) + @test ismissing(AG.getfield(f, 1)) # unset & null: N/A (but nothing otherwise) AG.unsetfield!(f, 1) - # Observe that OGRUnsetMarker and OGRNullMarkerare are mutually exclusive - @test !AG.isfieldset(f, 1) && !AG.isfieldnull(f, 1) # notice the field is notnull + # Observe that OGRUnset and OGRNull are mutually exclusive + @test !AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) # notice the field is notnull + + # setting the field for a notnullable column + AG.setnullable!(AG.getfielddefn(f, 1), false) + AG.setfield!(f, 1, "value") + @test AG.getfield(f, 1) == "value" + @test AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + AG.setfield!(f, 1, missing) + @test AG.getfield(f, 1) == AG.getdefault(f, 1) + @test AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + AG.setfield!(f, 1, nothing) + @test ismissing(AG.getfield(f, 1)) + @test !AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + + # setting the field for a nullable column + AG.setnullable!(AG.getfielddefn(f, 1), true) + AG.setfield!(f, 1, "value") + @test AG.getfield(f, 1) == "value" + @test AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + AG.setfield!(f, 1, missing) + @test ismissing(AG.getfield(f, 1)) + @test AG.isfieldset(f, 1) + @test AG.isfieldnull(f, 1) # different from that of notnullable + AG.setfield!(f, 1, nothing) + @test ismissing(AG.getfield(f, 1)) + @test !AG.isfieldset(f, 1) + @test !AG.isfieldnull(f, 1) + end end end From 763d83904a1c779b8ac3fb534575eb582f573c0d Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 23:22:05 -0700 Subject: [PATCH 07/14] Update the documentation to indicate the methods being discussed --- docs/src/considerations.md | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/docs/src/considerations.md b/docs/src/considerations.md index 77329e8b..1c13c1b7 100644 --- a/docs/src/considerations.md +++ b/docs/src/considerations.md @@ -98,7 +98,7 @@ The interface is implemented in [`src/tables.jl`](https://github.com/yeesian/Arc ## Missing and Null Semantics in GDAL -When reading the fields of a feature, ArchGDAL observes the following behavior: +When reading the fields of a feature using `getfield(feature, i)`, ArchGDAL observes the following behavior: | Field | null | notnull | |-------|---------|---------| @@ -106,14 +106,11 @@ When reading the fields of a feature, ArchGDAL observes the following behavior: | unset | nothing | missing | This reflects that -* a field that is notnull will never return `nothing`. - * use `isfieldnull(feature, i)` to determine if a field has been set. -* a field that is set will never return `nothing`. - * use `isfieldset(feature, i)` to determine if a field has been set. -* a field that is set and not null will always have a concrete value. - * use `isfieldsetandnotnull(feature, i)` to test for it. - -When writing the fields of a feature, ArchGDAL observes the following behavior: +* a field that is notnull will never return `nothing`: use `isfieldnull(feature, i)` to determine if a field has been set. +* a field that is set will never return `nothing`: use `isfieldset(feature, i)` to determine if a field has been set. +* a field that is set and not null will always have a concrete value: use `isfieldsetandnotnull(feature, i)` to test for it. + +When writing the fields of a feature using `setfield!(feature, i, value)`, ArchGDAL observes the following behavior: | Field | nullable | notnullable | |---------|----------|--------------| From 63b6f98cd596a99ce62efc9e62352be6dd5a9f5d Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 15 Sep 2021 23:33:30 -0700 Subject: [PATCH 08/14] remove assertions --- src/ogr/feature.jl | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 135692ef..40704461 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -464,18 +464,14 @@ const _FETCHFIELD = Dict{OGRFieldType,Function}( """ function getfield(feature::Feature, i::Integer) return if isfieldsetandnotnull(feature, i) - @assert isfieldset(feature, i) && !isfieldnull(feature, i) _fieldtype = getfieldtype(getfielddefn(feature, i)) _fetchfield = get(_FETCHFIELD, _fieldtype, getdefault) _fetchfield(feature, i) elseif isfieldset(feature, i) - @assert isfieldnull(feature, i) missing elseif isfieldnull(feature, i) - @assert !isfieldset(feature, i) nothing else - @assert !isfieldset(feature, i) && !isfieldnull(feature, i) missing end end From 08ffc271a64de12f352d12cb0eb4de3426ef1518 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Sat, 25 Sep 2021 15:48:51 -0700 Subject: [PATCH 09/14] Map unset fields to `nothing` and null values to `missing` Because getdefault() is meant to be used only when setting fields for notnullable columns with missing values, we make it return `nothing` instead of `missing` to unset the field. --- docs/src/considerations.md | 8 ++++---- src/ogr/feature.jl | 9 +++++---- src/ogr/fielddefn.jl | 4 ++-- test/test_feature.jl | 12 ++++++------ test/test_fielddefn.jl | 2 +- test/test_tables.jl | 34 +++++++++++++++------------------- 6 files changed, 33 insertions(+), 36 deletions(-) diff --git a/docs/src/considerations.md b/docs/src/considerations.md index 1c13c1b7..5c3535ac 100644 --- a/docs/src/considerations.md +++ b/docs/src/considerations.md @@ -103,11 +103,11 @@ When reading the fields of a feature using `getfield(feature, i)`, ArchGDAL obse | Field | null | notnull | |-------|---------|---------| | set | missing | value | -| unset | nothing | missing | +| unset | nothing | nothing | This reflects that -* a field that is notnull will never return `nothing`: use `isfieldnull(feature, i)` to determine if a field has been set. -* a field that is set will never return `nothing`: use `isfieldset(feature, i)` to determine if a field has been set. +* a field that is notnull will never return `missing`: use `isfieldnull(feature, i)` to determine if a field has been set. +* a field is set will never return `nothing` (and a field that unset will always return `nothing`): use `isfieldset(feature, i)` to determine if a field has been set. * a field that is set and not null will always have a concrete value: use `isfieldsetandnotnull(feature, i)` to test for it. When writing the fields of a feature using `setfield!(feature, i, value)`, ArchGDAL observes the following behavior: @@ -120,7 +120,7 @@ When writing the fields of a feature using `setfield!(feature, i, value)`, ArchG This reflects that * writing `nothing` will cause the field to be unset. -* writing `missing` will cause the field to be null. In the cause of a notnullable field, it will take the default value (see https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html for details). +* writing `missing` will cause the field to be null. In the cause of a notnullable field, it will take the default value (see https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html for details). If there is no default value, `getdefault()` will return `nothing`, causing the field to be unset. * writing a value will behave in the usual manner. For additional references, see diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 40704461..b51d3ce8 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -434,7 +434,7 @@ end # pfSecond,pnTZFlag) # end -function getdefault(feature::Feature, i::Integer)::Union{String,Missing} +function getdefault(feature::Feature, i::Integer)::Union{String,Nothing} return getdefault(getfielddefn(feature, i)) end @@ -458,6 +458,9 @@ const _FETCHFIELD = Dict{OGRFieldType,Function}( """ getfield(feature, i) +When the field is unset, it will return `nothing`. When the field is set but +null, it will return `missing`. + ### References * https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html * https://gdal.org/development/rfc/rfc67_nullfieldvalues.html @@ -469,10 +472,8 @@ function getfield(feature::Feature, i::Integer) _fetchfield(feature, i) elseif isfieldset(feature, i) missing - elseif isfieldnull(feature, i) - nothing else - missing + nothing end end diff --git a/src/ogr/fielddefn.jl b/src/ogr/fielddefn.jl index 33f513b7..2e964b7d 100644 --- a/src/ogr/fielddefn.jl +++ b/src/ogr/fielddefn.jl @@ -256,11 +256,11 @@ Get default field value ### References * https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html """ -function getdefault(fielddefn::AbstractFieldDefn)::Union{String,Missing} +function getdefault(fielddefn::AbstractFieldDefn)::Union{String,Nothing} result = @gdal(OGR_Fld_GetDefault::Cstring, fielddefn.ptr::GDAL.OGRFieldDefnH) return if result == C_NULL - missing + nothing else unsafe_string(result) end diff --git a/test/test_feature.jl b/test/test_feature.jl index d165d4fe..40516d03 100644 --- a/test/test_feature.jl +++ b/test/test_feature.jl @@ -91,7 +91,7 @@ const AG = ArchGDAL; true @testset "Missing and Null Semantics" begin - @test ismissing(AG.getdefault(f, 1)) + @test isnothing(AG.getdefault(f, 1)) AG.setdefault!(AG.getfielddefn(f, 1), "default value") @test AG.getdefault(f, 1) == "default value" @@ -103,12 +103,12 @@ const AG = ArchGDAL; AG.unsetfield!(f, 1) @test !AG.isfieldset(f, 1) @test !AG.isfieldnull(f, 1) # carried over from earlier - @test ismissing(AG.getfield(f, 1)) + @test isnothing(AG.getfield(f, 1)) # unset & notnull: missing AG.fillunsetwithdefault!(f) # nothing has changed - @test ismissing(AG.getfield(f, 1)) + @test isnothing(AG.getfield(f, 1)) # because it is a nullable field @test AG.isnullable(AG.getfielddefn(f, 1)) # even though it is not a null value @@ -129,7 +129,7 @@ const AG = ArchGDAL; AG.unsetfield!(f, 1) @test !AG.isfieldnull(f, 1) @test !AG.isfieldset(f, 1) - @test ismissing(AG.getfield(f, 1)) + @test isnothing(AG.getfield(f, 1)) # and we fill unset with default again AG.fillunsetwithdefault!(f) # the field is set to the default @@ -160,7 +160,7 @@ const AG = ArchGDAL; @test AG.isfieldset(f, 1) @test !AG.isfieldnull(f, 1) AG.setfield!(f, 1, nothing) - @test ismissing(AG.getfield(f, 1)) + @test isnothing(AG.getfield(f, 1)) @test !AG.isfieldset(f, 1) @test !AG.isfieldnull(f, 1) @@ -175,7 +175,7 @@ const AG = ArchGDAL; @test AG.isfieldset(f, 1) @test AG.isfieldnull(f, 1) # different from that of notnullable AG.setfield!(f, 1, nothing) - @test ismissing(AG.getfield(f, 1)) + @test isnothing(AG.getfield(f, 1)) @test !AG.isfieldset(f, 1) @test !AG.isfieldnull(f, 1) diff --git a/test/test_fielddefn.jl b/test/test_fielddefn.jl index 8a97e3a8..91faacba 100644 --- a/test/test_fielddefn.jl +++ b/test/test_fielddefn.jl @@ -59,7 +59,7 @@ const AG = ArchGDAL; AG.setnullable!(fd, true) @test AG.isnullable(fd) == true - @test ismissing(AG.getdefault(fd)) + @test isnothing(AG.getdefault(fd)) AG.setdefault!(fd, "0001/01/01 00:00:00") @test AG.getdefault(fd) == "0001/01/01 00:00:00" @test AG.isdefaultdriverspecific(fd) == true diff --git a/test/test_tables.jl b/test/test_tables.jl index bcb5682b..db5a9a5d 100644 --- a/test/test_tables.jl +++ b/test/test_tables.jl @@ -218,7 +218,7 @@ using Tables (i, i + 1) for i in 3.0:5.0 ]), ), - emptygeom = (name = "emptygeom", geom = nothing), + emptygeom = (name = "emptygeom", geom = missing), emptyfield = ( name = "emptyid", geom = AG.createlinestring([ @@ -250,7 +250,7 @@ using Tables (-1.0, -1.0), ]), ), - emptygeom = (name = "emptygeom", geom = nothing), + emptygeom = (name = "emptygeom", geom = missing), emptyfield = ( name = "emptyid", geom = AG.createpolygon([ @@ -334,7 +334,7 @@ using Tables end if withmissingfield AG.addfeature(newlayer) do newfeature - # No Id field set + AG.setfieldnull!(newfeature, id_idx) AG.setfield!( newfeature, name_idx, @@ -408,17 +408,13 @@ using Tables end # Helper functions - function toWKT_withmissings(x) - if ismissing(x) - return missing - elseif typeof(x) <: AG.AbstractGeometry - return AG.toWKT(x) - else - return x - end - end - function columntablevalues_toWKT(x) - return Tuple(toWKT_withmissings.(x[i]) for i in 1:length(x)) + wellknownvalue(obj::Any) = obj + wellknownvalue(obj::AG.AbstractGeometry) = AG.toWKT(obj) + wellknownvalue(obj::AG.AbstractSpatialRef) = AG.toWKT(obj) + wellknownvalue(obj::Missing)::Missing = missing + wellknownvalue(obj::Nothing)::Nothing = nothing + function wellknownvalues(x)::Tuple + return Tuple(wellknownvalue.(x[i]) for i in 1:length(x)) end tupleoftuples_equal = ( (x, y) -> @@ -461,7 +457,7 @@ using Tables return ( names = keys(Tables.columntable(layer)), types = eltype.(values(Tables.columntable(layer)),), - values = columntablevalues_toWKT( + values = wellknownvalues( values(Tables.columntable(layer)), ), ) @@ -500,7 +496,7 @@ using Tables @test keys(Tables.columntable(layer)) == reference_geotable.names @test eltype.(values(Tables.columntable(layer))) == reference_geotable.types @test tupleoftuples_equal( - columntablevalues_toWKT( + wellknownvalues( values(Tables.columntable(layer)), ), reference_geotable.values, @@ -524,7 +520,7 @@ using Tables @test keys(Tables.columntable(layer)) == reference_geotable.names @test eltype.(values(Tables.columntable(layer))) == reference_geotable.types @test tupleoftuples_equal( - columntablevalues_toWKT( + wellknownvalues( values(Tables.columntable(layer)), ), reference_geotable.values, @@ -733,14 +729,14 @@ using Tables @testset "Conversion to table for FlatGeobuf driver" begin FlatGeobuf_test_reference_geotable = ( names = (Symbol(""), :id, :name), - types = (ArchGDAL.IGeometry, Union{Missing,Int64}, String), + types = (ArchGDAL.IGeometry, Union{Nothing,Int64}, String), values = ( [ "LINESTRING (5 6,6 7,7 8)", "MULTILINESTRING ((1 2,2 3,3 4,4 5),(6 7,7 8,8 9,9 10))", "LINESTRING (1 2,2 3,3 4)", ], - Union{Missing,Int64}[missing, 2, 1], + Union{Nothing,Int64}[nothing, 2, 1], ["emptyid", "multiline1", "line1"], ), ) From 364f171a390f1073810e0cef3f2f0852e4be7e34 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Sun, 26 Sep 2021 11:16:50 -0700 Subject: [PATCH 10/14] Turn references into hyperlinks --- docs/src/considerations.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/src/considerations.md b/docs/src/considerations.md index 5c3535ac..f142f940 100644 --- a/docs/src/considerations.md +++ b/docs/src/considerations.md @@ -124,6 +124,6 @@ This reflects that * writing a value will behave in the usual manner. For additional references, see -* https://docs.julialang.org/en/v1/manual/faq/#faq-nothing -* https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html -* https://gdal.org/development/rfc/rfc67_nullfieldvalues.html +* [JuliaLang: Nothingness and missing values](https://docs.julialang.org/en/v1/manual/faq/#faq-nothing) +* [GDAL: OGR not-null constraints and default values](https://gdal.org/development/rfc/rfc53_ogr_notnull_default.html) +* [GDAL: Null values in OGR](https://gdal.org/development/rfc/rfc67_nullfieldvalues.html) From 323cbdb7719a1ad62affec6d0462fd0de7d18d7b Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Sun, 26 Sep 2021 13:42:00 -0700 Subject: [PATCH 11/14] use isfieldset() and isfieldnull() when getting a field isfieldsetandnotnull() is just a convenience function. --- src/ogr/feature.jl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index b51d3ce8..5d003cff 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -466,14 +466,14 @@ null, it will return `missing`. * https://gdal.org/development/rfc/rfc67_nullfieldvalues.html """ function getfield(feature::Feature, i::Integer) - return if isfieldsetandnotnull(feature, i) + return if !isfieldset(feature, i) + nothing + elseif isfieldnull(feature, i) + missing + else _fieldtype = getfieldtype(getfielddefn(feature, i)) _fetchfield = get(_FETCHFIELD, _fieldtype, getdefault) _fetchfield(feature, i) - elseif isfieldset(feature, i) - missing - else - nothing end end From 5c7b677e5350fe581a1e5ec9559d9ac91d302908 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Sun, 26 Sep 2021 13:44:23 -0700 Subject: [PATCH 12/14] raise an error instead of getting default if we do not recognize the fieldtype when reading the field (in the future, we will switch to dispatch on the fieldtype rather than the current dictionary approach, but that is out of the scope of this commit.) --- src/ogr/feature.jl | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 5d003cff..37473b81 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -472,8 +472,16 @@ function getfield(feature::Feature, i::Integer) missing else _fieldtype = getfieldtype(getfielddefn(feature, i)) - _fetchfield = get(_FETCHFIELD, _fieldtype, getdefault) - _fetchfield(feature, i) + try + _fetchfield = _FETCHFIELD[_fieldtype] + _fetchfield(feature, i) + catch e + if e isa KeyError + error("$_fieldtype not implemented in getfield, please report an issue to https://github.com/yeesian/ArchGDAL.jl/issues") + else + rethrow(e) + end + end end end From 092bed4a4ddaf4d08696ddbf4c2d655f1e0b24c5 Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Sun, 26 Sep 2021 13:49:00 -0700 Subject: [PATCH 13/14] Fix docstring for isfieldsetandnotnull() --- src/ogr/feature.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ogr/feature.jl b/src/ogr/feature.jl index 37473b81..e07aef97 100644 --- a/src/ogr/feature.jl +++ b/src/ogr/feature.jl @@ -177,7 +177,7 @@ Test if a field is set and not null. * `i`: the field to test, from 0 to GetFieldCount()-1. ### Returns -`true` if the field is null, otherwise `false`. +`true` if the field is set and not null, otherwise `false`. ### References * https://gdal.org/development/rfc/rfc67_nullfieldvalues.html From a7710c24a4101e10f79d79cc01ef6ae8fb09623e Mon Sep 17 00:00:00 2001 From: Yeesian Ng Date: Wed, 29 Sep 2021 21:07:59 -0700 Subject: [PATCH 14/14] Update to reflect NA for reading unset & null fields --- docs/src/considerations.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/src/considerations.md b/docs/src/considerations.md index f142f940..c8b4b4a1 100644 --- a/docs/src/considerations.md +++ b/docs/src/considerations.md @@ -100,10 +100,10 @@ The interface is implemented in [`src/tables.jl`](https://github.com/yeesian/Arc When reading the fields of a feature using `getfield(feature, i)`, ArchGDAL observes the following behavior: -| Field | null | notnull | -|-------|---------|---------| -| set | missing | value | -| unset | nothing | nothing | +| Field | null | notnull | +|-------|-----------|-----------| +| set | `missing` | value | +| unset | N/A | `nothing` | This reflects that * a field that is notnull will never return `missing`: use `isfieldnull(feature, i)` to determine if a field has been set. @@ -112,11 +112,11 @@ This reflects that When writing the fields of a feature using `setfield!(feature, i, value)`, ArchGDAL observes the following behavior: -| Field | nullable | notnullable | -|---------|----------|--------------| -| nothing | unset | unset | -| missing | null | getdefault() | -| value | value | value | +| Field | nullable | notnullable | +|-----------|----------|----------------| +| `nothing` | unset | unset | +| `missing` | null | `getdefault()` | +| value | value | value | This reflects that * writing `nothing` will cause the field to be unset.