Update to TileDB 2.29.0. #383
Conversation
| // Only objects will be traversed for cloud storage backends such as S3, Azure, and GCS. | ||
| // | ||
| // Deprecated: Use VisitRecursiveV2 instead. | ||
| func (v *VFS) VisitRecursive(path string, callback VisitRecursiveCallback) error { |
There was a problem hiding this comment.
We should reimplement VisitRecursive by calling VisitRecursiveV2 and ignoring the last parameter. This is what I did in C#. You will need some more logic to handle the callback returning false, nil; likely return and check for a sentinel error value.
There was a problem hiding this comment.
🤔 At first it SGTM, but wouldn't this force users into the new behavior of traversing prefix results for cloud backends on the V1 API? If I hack around that to preserve the V1 behavior while still calling VisitRecursiveV2, we'd still need to deprecate VisitRecursive based on that behavior and the end result would be the same?
There was a problem hiding this comment.
Oh, V1 keeps the old behavior.
If you ask me, the old behavior of tiledb_vfs_ls_recursive is broken to the point of being unusable, and I cannot imagine anyone depending on this in production code. Instead of ignoring isDir, we can drop directory results in VisitRecursive-over-VisitRecursiveV2. This is technically a breaking change, but reduces duplication in the implementation, and makes VisitRecursive's behavior consistent.
There was a problem hiding this comment.
I think we've had this discussion in slack? https://tiledb.slack.com/archives/C015Z7QG6BU/p1754039626144409
Core would have been the place to make this change, instead of each downstream API
I don't think that we should make a breaking change to an API that has been deprecated in core within the same release. The code duplication will be removed at the end of the deprecation cycle. That said I'm happy to do this if others agree.
We could easily write helpers like VisitRecursiveObjects / VisitRecursiveDirectories without breaking clients, but I don't see a need to modify the deprecated V1 API behavior if we have a V2 API and it's available.
| dirExists, err := vfs.IsDir(path) | ||
| t.Run("VisitRecursive", func(t *testing.T) { | ||
| var fileList []string | ||
| err = vfs.VisitRecursive(tmpPath, func(path string, size uint64) (bool, error) { |
There was a problem hiding this comment.
No need to test both VisitRecursive and VisitRecursiveV2, if the former is implemented in terms of the latter.
Co-authored-by: Theodore Tsirpanis <teo@tsirpanis.gr>
teo-tsirpanis
deleted the
shaunreed/cloud-1999-add-tiledb-go-binding-for-ls_recursive-v2
branch
2 participants
This updates to TileDB 2.29.0 and wraps
tiledb_vfs_ls_recursive_v2for use in TileDB-Go.