yocontra / node-gdal-next

Node.js bindings for GDAL (Geospatial Data Abstraction Library) [Fork]
https://contra.io/node-gdal-next/
Apache License 2.0
75 stars 36 forks source link

Field type support for int64 #14

Closed warrenwyf closed 3 years ago

warrenwyf commented 3 years ago

https://contra.io/node-gdal-next/classes/Constants%20(OFT).html#prop-gdal.OFTInteger

There is no field type OFTInteger64 and OFTInteger64List which gdal has:

https://gdal.org/doxygen/ogr__core_8h.html#a787194bea637faf12d61643124a7c9fc

yocontra commented 3 years ago

@warrenwyf We have code for OFTInteger64 here that is enabled on GDAL >= 2.x: https://github.com/contra/node-gdal-next/blob/master/src/collections/feature_fields.cpp#L404

yocontra commented 3 years ago

So it looks like the gap we have is:

@warrenwyf Does solving those two fully address your issue or are there more cases we need to handle?

warrenwyf commented 3 years ago

So it looks like the gap we have is:

  • We don't have any code for parsing OFTInteger64List in FeatureFields (OFTInteger64 is handled already)
  • We don't expose the type constants for OFTInteger64 or OFTInteger64List

@warrenwyf Does solving those two fully address your issue or are there more cases we need to handle?

Yes, I think so

yocontra commented 3 years ago

@warrenwyf Do you have a sample file that includes those two data types that I can use to test? Will make it much easier to implement if I have something to work against

warrenwyf commented 3 years ago

@warrenwyf Do you have a sample file that includes those two data types that I can use to test? Will make it much easier to implement if I have something to work against

Integer64List is not supported by most formats, maybe you can test it in this way:

  const dataset = gdal.open(require.resolve('./test.gpkg'), 'w', 'GPKG');
  const layer = dataset.layers.create('test', null, gdal.wkbPoint);

  layer.fields.add(new gdal.FieldDefn('bigintlist', gdal.OFTInteger64List));

  const feature = new gdal.Feature(layer);
  feature.fields.set('bigintlist', [1,2,3]);
  layer.features.add(feature);

  console.log(feature)

  dataset.close();
yocontra commented 3 years ago

Should be solved with this (https://github.com/contra/node-gdal-next/commit/6ed2a1988e484a52f20b45dd554277140105b747) published as 2.2.0 - let me know if this solves your issue and I can close out the ticket. Waiting for the build to finish and will publish to NPM in the morning.

yocontra commented 3 years ago

Published as 2.2.0 now that the binary published - let me know if you have any issues with it.

warrenwyf commented 3 years ago

Published as 2.2.0 now that the binary published - let me know if you have any issues with it.

Seems works.

However, the data type to set is not correct:

feature.fields.set('bigintlist', [1, 2, 3]); // does not work
feature.fields.set('bigintlist', `[1, 2, 3]`); // works
yocontra commented 3 years ago

@warrenwyf JS doesn't have 64 bit integers so it makes sense that you would need to represent it in a string. Don't know of any other workaround - let me know if you have any ideas.

warrenwyf commented 3 years ago

@warrenwyf JS doesn't have 64 bit integers so it makes sense that you would need to represent it in a string. Don't know of any other workaround - let me know if you have any ideas.

but feature.fields.get('bigintlist') returns an array instead of string, the set()-get() behavior is inconsistent