ogrinfo() and SQL

[mdsumner]

I see that we can't get meaningful info when SQL query is applied

      Rcpp::Rcout << "DSN: " << m_dsn << std::endl;
        Rcpp::Rcout << "layer: \"" << m_layer_name << "\"" << std::endl;

I'm toying with changing it like this:

void GDALVector::info() const {
    checkAccess_(GA_ReadOnly);

   
#if GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(3, 7, 0)
   if (m_is_sql) {
     Rcpp::Rcout << ogrinfo(m_dsn, R_NilValue,
                            Rcpp::CharacterVector::create("-so", "-nomd", "-sql", m_layer_name),
                            m_open_options, true, false);
   }
   else {
        Rcpp::Rcout << ogrinfo(m_dsn, Rcpp::wrap(m_layer_name),
                               Rcpp::CharacterVector::create("-so", "-nomd"),
                               m_open_options, true, false);
#else
        Rcpp::Rcout << "ogrinfo() requires GDAL >= 3.7" << std::endl;
        Rcpp::Rcout << "DSN: " << m_dsn << std::endl;
        Rcpp::Rcout << "layer: " << m_layer_name << std::endl;
#endif
    }
}

which seems to work. (I'm hoping to add infoAsJSON as well). Just wanted to ping in case that kind of change concerns you via things I'm not considering?

[ctoney]

Nice, I cant't find anything that would be a problem with that.

I see now that we could just write a regular if (GDAL_VERSION_NUM < GDAL_COMPUTE_VERSION(3, 7, 0) ... else instead of #if ... #else ... #endif since there is nothing in that block that would fail to compile with GDAL < 3.7. ogrinfo() itself already has the conditional compilation that avoids trying to call GDALVectorInfo() if GDAL < 3.7. But not a critical change, either will work.

[mdsumner]

Oh I.see on the version thing 🙏, happy to add that to PR if you like

I wondered if GDAL allowed info on an open layer, but doesn't seem to ? Maybe I need to look more closely

[ctoney]

I wondered if GDAL allowed info on an open layer, but doesn't seem to ?

Not sure exactly what you mean but good question. I'm assuming we can call ogrinfo() / GDALVectorInfo() on an open dataset/layer since that is what GDALVector::info() is doing. The call to ogrinfo() opens a new read-only GDALDataset on the C++ side, gets info on the layer and then releases that dataset object so drops a reference to the underlying data source. It should be okay that there is another GDALDataset (in the open GDALVector object in R) that also has a reference to the data source.

Are you seeing anything different? Our tests are not especially robust since it's only tested with GPKG. Should we test with other formats?

[mdsumner]

I think what I'm getting at is that (in gdal_exp.cpp) translate() has signature

bool translate(const GDALRaster* const &src_ds,

providing the active pointers to an open dataset.

but ogrinfo() "starts from scratch" by opening up a new dataset as you said. It could be rejigged to work on the open dataset (but I'm not really sure if that's valid either, because the open info-options used is not in the same form as that required by GDALVectorInfo).

I get as_json for free on (my draft) GDALVector$infoAsJSON() because you already implemented ogrinfo(), but I might as well just use gdalraster::ogrinfo() anyway.

I'm totally happy to go with the setup as is now, this is really good familiarization for me - just I think I'm seeing a grander refactor that could happen. (gdalraster::translate() is implemented for standalone use by spinning up a new GDALRaster, but an existing object can do GDALRaster$translate() with no spin up).

Does that make sense? I don't want to derail anything or push for a big change, but I might start exploring that bigger picture. Maybe there's a reason ogrinfo() can't be approached that way?

[mdsumner]

certainly I don't see problems with other formats, it's working on everything I try (shp in zip urls, WFS, Parquet etc)

[ctoney]

Does that make sense?

Yes I see what you're getting at. Please note this partially relates to recent change for GDALRaster in #606 and pending #609 for vector, just in the sense that passing those objects actually works as it should now... as references rather than by value which previously resulted in a copy of the object locally to the function scope when passed in. Anyway, that should all work properly now and I'm much more confident in how the objects are being passed in different scenarios (R side vs C++ side, and constructing from scratch in C++ and returning). Some refactor you're referring to could be worthwhile. Let me think through some more and reply in the next day or two with a little more detail. Thanks for looking at that.