EugeneYushin commented on PR #52759:
URL: https://github.com/apache/spark/pull/52759#issuecomment-3476418806
> In BasicInMemoryTableCatalog#initialize, we keep the passed options, so
that we can verify it in tests, to make sure it contains myKey, and its value
is empty string, not ${env:NON_EXISITING}
We can do the same in
[CatalogLoadingSuite.testInitializationOptions](https://github.com/EugeneYushin/spark/blob/46160ff7a6204a9c9a00e6f177b95deda3e00ab0/sql/catalyst/src/test/java/org/apache/spark/sql/connector/catalog/CatalogLoadingSuite.java#L28)
as well. The thing is that the current behavior of ConfigReader is to [fall
back to whatever has been
passed](https://github.com/EugeneYushin/spark/blob/fe904e6973b7a8fdadc5e253a6a74e8ccb359287/common/utils/src/main/scala/org/apache/spark/internal/config/ConfigReader.scala#L99)
if respective ConfigProvider returns None.
In this particular case with `${env:NON_EXISITING}` it returns the same
original `${env:NON_EXISITING}` string because
[EnvProvider](https://github.com/EugeneYushin/spark/blob/fe904e6973b7a8fdadc5e253a6a74e8ccb359287/common/utils/src/main/scala/org/apache/spark/internal/config/ConfigProvider.scala#L31)
can't find `NON_EXISITING` env var.
It makes sense to me to leave the aforementioned logic untouched as it's
kind of core functionality.
As for test coverage, this meanin we should provide expected sys.env (or
sys.pros, or maybe some other way to test it?!) for a particular test case.
I'll be happy to proceed with unit test coverage, but not sure what could be
the best way of doing this w/o touching `sys.env` which is no-go for obvious
reasons. Ideally we should be able to set ConfigReader instance (and override
it with implementation that can mock env var, same way it's done in
[ConfigReader
tests](https://github.com/EugeneYushin/spark/blob/b7763a7eae2b9609012cbb4ce981276c6471cc4e/core/src/test/scala/org/apache/spark/internal/config/ConfigReaderSuite.scala#L30-L31)),
but the enclosing `Catalogs` abstraction is singleton.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
