Skip to content
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

copy all array like feature fields after unsafe_wrap #442

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

asinghvi17
Copy link
Contributor

@asinghvi17 asinghvi17 commented Nov 13, 2024

Fixes evetion/GeoDataFrames.jl#86

This PR adds a copy around unsafe_wrap when getting vectors out of GDAL features. This guards against GDAL overwriting that memory, as we found in the issue.

Not sure how best to test that this works. A memory sweat test might crash the CI....

@evetion
Copy link
Collaborator

evetion commented Nov 13, 2024

FWIW, the documentation here was correct, so this might need fixing on a slightly higher level (the caller of these functions).

@asinghvi17
Copy link
Contributor Author

asinghvi17 commented Nov 13, 2024

This would probably be better handled in GDAL.aftercare? It already does this for strings so...
If we're sticking to GDAL API then yeah, that makes sense.

@asinghvi17
Copy link
Contributor Author

@alex-s-gardner you can ]add https://github.com/asinghvi17/ArchGDAL.jl#patch-3 to get this branch.

@alex-s-gardner
Copy link
Contributor

@alex-s-gardner you can ]add https://github.com/asinghvi17/ArchGDAL.jl#patch-3 to get this branch.

Testing now

@alex-s-gardner
Copy link
Contributor

I get this error when trying to open a parquet file:

ERROR: MethodError: no method matching copy(::Vector{Int64})
You may have intended to import Base.copy
The function `copy` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  copy(::Function, Any...; kwargs...)
   @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/context.jl:265
  copy(::ArchGDAL.AbstractFeatureLayer; dataset, name, options)
   @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/ogr/featurelayer.jl:73
  copy(::ArchGDAL.RasterDataset, Any...; kwargs...)
   @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/raster/array.jl:65
  ...

Stacktrace:
  [1] getfield(feature::ArchGDAL.IFeature, i::Int32)
    @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/ogr/feature.jl:543
  [2] getfield
    @ ~/.julia/packages/ArchGDAL/N1Ko0/src/ogr/feature.jl:561 [inlined]
  [3] getcolumn(row::ArchGDAL.IFeature, name::Symbol)
    @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/tables.jl:23
  [4] eachcolumns
    @ ~/.julia/packages/Tables/8p03y/src/utils.jl:127 [inlined]
  [5] _buildcolumns(rowitr::ArchGDAL.FeatureLayer, row::ArchGDAL.IFeature, st::Int64, sch::Tables.Schema{…}, columns::NTuple{…}, updated::Base.RefValue{…})
    @ Tables ~/.julia/packages/Tables/8p03y/src/fallbacks.jl:197
  [6] buildcolumns
    @ ~/.julia/packages/Tables/8p03y/src/fallbacks.jl:227 [inlined]
  [7] _columns
    @ ~/.julia/packages/Tables/8p03y/src/fallbacks.jl:265 [inlined]
  [8] columns
    @ ~/.julia/packages/Tables/8p03y/src/fallbacks.jl:258 [inlined]
  [9] DataFrame(x::ArchGDAL.FeatureLayer; copycols::Nothing)
    @ DataFrames ~/.julia/packages/DataFrames/kcA9R/src/other/tables.jl:57
 [10] DataFrame
    @ ~/.julia/packages/DataFrames/kcA9R/src/other/tables.jl:48 [inlined]
 [11] #15
    @ ~/.julia/packages/GeoDataFrames/eP9Sg/src/io.jl:90 [inlined]
 [12] getlayer(::GeoDataFrames.var"#15#16"{ArchGDAL.Dataset}, ::ArchGDAL.Dataset, ::Vararg{Any}; kwargs::@Kwargs{})
    @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/context.jl:268
 [13] getlayer
    @ ~/.julia/packages/ArchGDAL/N1Ko0/src/context.jl:265 [inlined]
 [14] read(ds::ArchGDAL.Dataset, layer::Int64)
    @ GeoDataFrames ~/.julia/packages/GeoDataFrames/eP9Sg/src/io.jl:80
 [15] (::GeoDataFrames.var"#10#11")(ds::ArchGDAL.Dataset)
    @ GeoDataFrames ~/.julia/packages/GeoDataFrames/eP9Sg/src/io.jl:62
 [16] read(f::GeoDataFrames.var"#10#11", args::String; kwargs::@Kwargs{})
    @ ArchGDAL ~/.julia/packages/ArchGDAL/N1Ko0/src/context.jl:268
 [17] read
    @ ~/.julia/packages/ArchGDAL/N1Ko0/src/context.jl:265 [inlined]
 [18] read(fn::String; kwargs::@Kwargs{})
    @ GeoDataFrames ~/.julia/packages/GeoDataFrames/eP9Sg/src/io.jl:58
 [19] read(fn::String)
    @ GeoDataFrames ~/.julia/packages/GeoDataFrames/eP9Sg/src/io.jl:52
 [20] top-level scope
    @ ~/Documents/GitHub/Altim.jl/src/glacier_routing.jl:179
Some type information was truncated. Use `show(err)` to see complete types.

src/ogr/feature.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@alex-s-gardner alex-s-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now working as expected

@asinghvi17
Copy link
Contributor Author

Test failures seem unrelated to this PR.

@yeesian yeesian merged commit 58e2c03 into yeesian:master Nov 14, 2024
1 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

!!!! erroneous table mutation when adding a large column !!!!
5 participants