[C++][FS][Azure] Should GetFileInfo() against a directory always return true without hierarchical namespace support?

[kou]

Describe the enhancement requested

Because CreateDir() without hierarchical namespace support does nothing and returns arrow::Status::OK().

See also the discussion: #38708 (comment)

Component(s)

C++

[felipecrv]

I thought a bit more about this idea and realized always returning OK() on "directory exists?" queries would contradict the post-condition of DeleteDir() – after DeleteDir() the caller expects DirectoryExists() to return false.

I think our approach of doing nothing on CreateDir has to be reconsidered. s3fs.cc creates an empty object with a path ending in / to indicate a directory was created:

  Status CreateEmptyDir(const std::string& bucket, const std::string& key) {
    DCHECK(!key.empty());
    return CreateEmptyObject(bucket, key + kSep);
  }

cc @Tom-Newton

[Tom-Newton]

Yeah, I saw the "directory markers" thing on the S3 filesystem. Does anyone know if that is an arrow thing or more common pattern with S3? Personally I assumed that was a common pattern in S3 and I'm not aware of the same pattern existing in Azure.

My only concern with this approach would be when listing blobs. We could handle it in the arrow filesystem but if anybody tries to list with a different blob storage client they might get a confusing result.

[Tom-Newton]

It looks like the arrow GCS filesystem also creates directory markers in the same way as S3

arrow/cpp/src/arrow/filesystem/gcsfs.cc

Line 407 in c1b12ca

google::cloud::StatusOr<gcs::ObjectMetadata> CreateDirMarker(const std::string& bucket,

. The azure fsspec filesystem though, does a noop for create directory https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L1060C6-L1060C6

If we chose to create directory markers it would probably be good to add some metadata to the blob to indicate that it's actually a directory. I believe hierarchical namespace accounts add hdi_isfolder but looking at the fsspec implementation it checks for several metadata fields https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L851. I think the arrow filesystem should be compatible with the fsspec one so we should use one of those metadata fields.

[felipecrv]

Personally I assumed that was a common pattern in S3 and I'm not aware of the same pattern existing in Azure.

We're trying to impose filesystem semantics on top of a blob store and filesystem semantics require that after a CreateDir and DeleteDir you get directory exists and directory doesn't exist outcomes respectively.

The azure fsspec filesystem though, does a noop for create directory https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L1060C6-L1060C6

This might not be very relevant here because the goal of the Arrow Filesystem API is to provide an uniform interface on top of regular filesystems and blob storage systems. It's pretty clear that the Azure fsspec is violating filesystem semantics of directory management in this case.

[felipecrv]

If we chose to create directory markers it would probably be good to add some metadata to the blob to indicate that it's actually a directory. I believe hierarchical namespace accounts add hdi_isfolder but looking at the fsspec implementation it checks for several metadata fields https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L851. I think the arrow filesystem should be compatible with the fsspec one so we should use one of those metadata fields.

If the Azure fsspec has ways of providing believable filesystem semantics, then we could adopt their patterns. The main thing is: we should provide filesystem semantics and no-op for CreateDir/DirExists would not achieve that.

[Tom-Newton]

I think I'm in agreement. If we want to be rigorous we need to add directory markers in CreateDir.

I think the current implementation of GetFileInfo will already return that a directory is present but probably we should make sure to add a test case for empty directory with just a directory marker.

Probably I should also have added checks for metadata indicating a blob is a directory marker on ObjectInputFile::Init and ObjectAppendStream::Init.

[kou]

@felipecrv @Tom-Newton Do either of you want to work on this?

[Tom-Newton]

@felipecrv @Tom-Newton Do either of you want to work on this?

I might be able to do it in about a week

[felipecrv]

@felipecrv @Tom-Newton Do either of you want to work on this?

I will take this one.