HyukjinKwon commented on code in PR #59932:
URL: https://github.com/apache/spark/pull/59932#discussion_r4417637113
##########
sql/core/src/test/resources/native-datasource/test_native_datasource.cc:
##########
@@ -648,14 +656,34 @@ thread_local std::string last_error;
// Spark, as spark_ds_init got it.
const SparkDsHost* host = nullptr;
-// Logs with the log of Spark. A library must only use the fields that fit in
the size of the
-// struct that Spark passes, which may be of another version of the header.
+// Whether the struct that Spark passes has is_log_enabled, which an earlier
version of the header
+// does not have.
+bool hasIsLogEnabled() {
+ return host->size >= static_cast<int64_t>(offsetof(SparkDsHost,
is_log_enabled) +
+ sizeof(host->is_log_enabled));
+}
+
+// Logs with the log of Spark, unless it does not write the level. A library
must only use the
+// fields that fit in the size of the struct that Spark passes, which may be
of another version of
+// the header.
void hostLog(int32_t level, const std::string& message) {
+ if (hasIsLogEnabled() && !host->is_log_enabled(level)) return;
Review Comment:
Done in 8b94b3b: the test logs with the option `log` while the logger is at
ERROR, and checks that no event has the message.
##########
sql/core/src/main/resources/org/apache/spark/sql/datasource/spark_datasource.h:
##########
@@ -284,6 +294,13 @@ typedef struct SparkDsHost {
* of the driver or of the executor, with the path of the library.
*/
void (*log)(int32_t level, const char* message);
+ /*
+ * Returns whether log writes the messages of one of the SPARK_DS_LOG_*
levels, so that the
+ * library only formats the messages that Spark writes, and gets those that
Spark writes at the
+ * levels that a library would not log by default, such as debug. It follows
the configuration of
+ * the log of Spark, which can change while Spark runs.
+ */
+ bool (*is_log_enabled)(int32_t level);
Review Comment:
Done in 8b94b3b: both comments say that another level is treated as
`SPARK_DS_LOG_INFO`. `is_log_enabled` is described as the level of the logger,
which a filter can still drop a message of, and as never NULL in a struct whose
size holds it.
##########
docs/sql-native-data-sources.md:
##########
@@ -70,7 +70,9 @@ A native data source library exports a single C function,
`spark_ds_init`, which
of function pointers, a `SparkDsLibrary`, that implements the data source: it
needs no JVM headers
or bindings. Each library has its own table, so any number of native data
sources can be loaded
side by side. When Spark loads a library, it calls `spark_ds_init` with a
`SparkDsHost`, which
-gives the library the version of Spark and a function to log messages to the
log of Spark.
+gives the library the version of Spark, a function to log messages to the log
of Spark, and a
+function that tells whether Spark logs the messages of a level, so that a
library follows the log
+level of Spark.
Review Comment:
Done in 8b94b3b: the guide has a sentence on the order of the calls next to
the other conventions.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/v2/ffi/NativeLibrary.java:
##########
@@ -491,6 +546,25 @@ private static void log(String path, int level, long
message) {
}
}
+ /**
+ * Called by the library, through {@code SparkDsHost.is_log_enabled}, with
the level of the
+ * messages of {@link #log}, which logs the levels that it does not know as
info. It must not
+ * throw: an exception thrown by an upcall crashes the JVM.
+ */
+ private static boolean isLogEnabled(int level) {
+ try {
+ return switch (level) {
Review Comment:
Done in 8b94b3b: `log` and `isLogEnabled` share a `LogLevel` enum for the
mapping.
##########
sql/core/src/main/resources/org/apache/spark/sql/datasource/spark_datasource.h:
##########
@@ -284,6 +294,13 @@ typedef struct SparkDsHost {
* of the driver or of the executor, with the path of the library.
*/
void (*log)(int32_t level, const char* message);
+ /*
+ * Returns whether log writes the messages of one of the SPARK_DS_LOG_*
levels, so that the
+ * library only formats the messages that Spark writes, and gets those that
Spark writes at the
+ * levels that a library would not log by default, such as debug. It follows
the configuration of
+ * the log of Spark, which can change while Spark runs.
Review Comment:
Done in 8b94b3b: the comment says that each call goes into the JVM, and that
a library may keep the answer for a while and ask again from time to time.
--
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]
