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

TileDB: migrate away from deprecated APIs #9725

Merged
merged 11 commits into from
Apr 26, 2024
Merged

TileDB: migrate away from deprecated APIs #9725

merged 11 commits into from
Apr 26, 2024

Conversation

teo-tsirpanis
Copy link
Contributor

What does this PR do?

This PR updates the TileDB backend to stop using deprecated TileDB APIs. The changes are straightforward and can be divided into three categories:

  • query.set_buffer(name, offsets, data)query.set_data_buffer(name, data); query.set_offsets_buffer(name, offsets)
    • offsets and data are usually std::vectors, but in some cases were pairs of pointers and lengths
  • tiledb::Array(ctx, path, mode, timestamp)tiledb::Array(ctx, path, mode, tiledb::TemporalPolicy(tiledb::TimeTravel, timestamp))
  • query.set_subarray(subarray)tiledb::Subarray subarray_obj(ctx, array); subarray_obj.set_subarray(subarray); query.set_subarray(subarray_obj);

Validated by successfully building the GDAL library with a special build of TileDB that excludes all deprecated APIs (TileDB-Inc/TileDB#4887).

What are related issues/pull requests?

Tasklist

  • Make sure code is correctly formatted (cf pre-commit configuration) — ran clang-format on all files I changed
  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@rouault
Copy link
Member

rouault commented Apr 22, 2024

@teo-tsirpanis Thanks. I was just starting working on other TileDB updates coordinated by @perrygeo.
Our current minimum was TileDB 2.7. I see with this update it now seems to be 2.15 (between 2.7 and 2.15, the main build error seems to be related with tiledb::TemporalPolicy(tiledb::TimeTravel, nTimestamp)). Let's see what our CI thinks about it. Any thoughts on if an updated baseline to 2.15 is OK ?

@rouault
Copy link
Member

rouault commented Apr 22, 2024

You may also want to include the following changes:

$ git diff ../frmts/tiledb/CMakeLists.txt ../frmts/tiledb/include_tiledb.h
diff --git a/frmts/tiledb/CMakeLists.txt b/frmts/tiledb/CMakeLists.txt
index 2b68b39f7e..86fc626591 100644
--- a/frmts/tiledb/CMakeLists.txt
+++ b/frmts/tiledb/CMakeLists.txt
@@ -25,5 +25,4 @@ endif()
 gdal_standard_includes(gdal_TileDB)
 target_include_directories(gdal_TileDB PRIVATE
                            ${GDAL_RASTER_FORMAT_SOURCE_DIR}/mem)
-target_compile_definitions(gdal_TileDB PRIVATE -DTILEDB_DEPRECATED=)
 gdal_target_link_libraries(gdal_TileDB PRIVATE TileDB::tiledb_shared)
diff --git a/frmts/tiledb/include_tiledb.h b/frmts/tiledb/include_tiledb.h
index f721cb3c59..cec9844846 100644
--- a/frmts/tiledb/include_tiledb.h
+++ b/frmts/tiledb/include_tiledb.h
@@ -35,11 +35,6 @@
 #pragma GCC system_header
 #endif
 
-#ifdef _MSC_VER
-#pragma warning(push)
-#pragma warning(disable : 4996) /* XXXX was deprecated */
-#endif
-
 #ifdef INCLUDE_ONLY_TILEDB_VERSION
 #include "tiledb/tiledb_version.h"
 #else
@@ -47,10 +42,6 @@
 #include "tiledb/tiledb_experimental"
 #endif
 
-#ifdef _MSC_VER
-#pragma warning(pop)
-#endif
-
 #if TILEDB_VERSION_MAJOR > 2 ||                                                \
     (TILEDB_VERSION_MAJOR == 2 && TILEDB_VERSION_MINOR >= 9)
 #define HAS_TILEDB_GROUP

@rouault
Copy link
Member

rouault commented Apr 22, 2024

@teo-tsirpanis so, the idea is to remove those deprecated TileDB deprecated API in a future TileDB version ? When that will be ? Because GDAL 3.9 is soon to be released (it has been branched off a few hours ago from master) and will have a lifetime of about 6 months. So if a TileDB version without the deprecated API is going to land soon, it means that we will have to backport those changes to 3.9 too and raise the minimum to TileDB 2.15

@rouault
Copy link
Member

rouault commented Apr 22, 2024

@teo-tsirpanis I've added in your branch a commit fixing a runtime crash in the tests, and another one to implement #9725 (comment)

@teo-tsirpanis
Copy link
Contributor Author

Thanks!

@coveralls
Copy link
Collaborator

coveralls commented Apr 23, 2024

Coverage Status

coverage: 69.058% (+0.002%) from 69.056%
when pulling 848608b on teo-tsirpanis:tiledb-deprecated
into d14f23c on OSGeo:master.

@rouault
Copy link
Member

rouault commented Apr 23, 2024

I've added a few follow-up cleanup commits due to TileDB >= 2.15 being required

@ihnorton
Copy link

@rouault we just recently added this infrastructure in the build system to disable deprecated APIs, as a transitional step to actually removing them. The tentative hope was to:

  1. make as many fixes ourselves as we can (eg this PR)
  2. disable such APIs by default in the June release (make them opt-in with a build config option)
  3. actually remove in Aug-Sep timeframe, depending on how much feedback we get after (2)

Given GDAL's usage and lifecycle, we can be flexible about point (2) and delay removal until end-of-year if it is not possible to backport this within the 3.9 lifecycle (I can understand that such a change will make conda and other packaging much more difficult).

(cc @KiterLuc)

@rouault
Copy link
Member

rouault commented Apr 24, 2024

@ihnorton I don't really anticipate packaging issues by requiring TileDB >= 2.15, as most people that will package GDAL 3.9 will probably use up to date TileDB too. Conda-forge already ships TileDB 2.22 . Alpine Linux is a 2.17.4 (https://pkgs.alpinelinux.org/packages?name=tiledb&branch=edge&repo=&arch=&maintainer=).
I can probe opinions on the gdal-dev mailing list about that to see if that would cause an issue.

@rouault
Copy link
Member

rouault commented Apr 26, 2024

ok, merging this, and backporting it to 3.9 in time for 3.9.0

@rouault rouault merged commit 512003c into OSGeo:master Apr 26, 2024
33 checks passed
@teo-tsirpanis teo-tsirpanis deleted the tiledb-deprecated branch April 26, 2024 14:08
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.

4 participants