`GOOGLESQL_QCHECK_OK` compiles out its argument under `NDEBUG`
- Dominant language
- C++
- Stars
- 2.6k
- Forks
- 260
- PR merge metrics
- No merged PRs in 30d
Description
Optimized (`-c opt`, i.e. `-DNDEBUG`) builds of the analyzer constant-fold **every** `CAST( AS DATETIME)` to the zero datetime:
```sql
SELECT CAST(CAST(DATETIME "2006-01-02" AS DATE) AS DATETIME)
-- -c opt: 1970-01-01 00:00:00 (wrong, for every input)
-- fastbuild: 2006-01-02 00:00:00 (correct)
```
This is since [googlesql/base/status.h](https://github.com/google/googlesql/blob/1f8aa333f4d6353cd3a64471fc83121df72df3f7/googlesql/base/status.h#L29-L30) defines the `QCHECK` variant with a `DCHECK` body - which does not evaluate its arguments:
```cpp
#define GOOGLESQL_QCHECK_OK(val) \
ABSL_DCHECK_EQ(::absl::OkStatus(), ::googlesql::status_internal::AsStatus((val)))
```
See [QCHECK](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h#L60-L66) and [DCHECK](https://github.com/abseil/abseil-cpp/blob/master/absl/log/check.h#L83-L89) in abseil.
`QCHECK` semantics require the condition to be evaluated in **all** build modes. The only significant victim is the [DATE→DATETIME cast in googlesql/public/cast.cc](https://github.com/google/googlesql/blob/1f8aa333f4d6353cd3a64471fc83121df72df3f7/googlesql/public/cast.cc#L1351-L1356), where the entire conversion is the macro argument:
```cpp
case FCT(TYPE_DATE, TYPE_DATETIME): {
DatetimeValue datetime; // default ctor = 1970-01-01 00:00:00
GOOGLESQL_QCHECK_OK(
functions::ConstructDatetime(v.date_value(), TimeValue(), &datetime));
return Value::Datetime(datetime); // NDEBUG: returns the untouched default
}
```
With `-DNDEBUG` the `ConstructDatetime` call is compiled out and `datetime` is never written.
Possible fix:
```patch
From 7a9aad8b0cddb877362edf580f6b55ceacea6b72 Mon Sep 17 00:00:00 2001
From: Fredrik Fornwall
Date: Thu, 23 Jul 2026 04:04:09 +0200
Subject: [PATCH] Fix GOOGLESQL_QCHECK_OK silently compiling out its argument
under NDEBUG
GOOGLESQL_QCHECK_OK was defined as ABSL_DCHECK_EQ, whose arguments are
not evaluated when NDEBUG is defined (absl expands DCHECKs to a
`while (false && ...)` no-op in release builds). QCHECK semantics
require the condition to be evaluated in all build modes.
This made the analyzer's DATE->DATETIME constant fold in cast.cc return
the default-constructed DatetimeValue (1970-01-01 00:00:00) for every
input in optimized (-DNDEBUG) builds: the entire ConstructDatetime()
call was the macro argument, so it was compiled out and the output
variable was never written.
Fix:
- Define GOOGLESQL_QCHECK_OK with ABSL_QCHECK_EQ (always evaluated),
and include/depend on absl_check explicitly.
- Switch the cast.cc DATE->DATETIME case to GOOGLESQL_RETURN_IF_ERROR,
matching every sibling conversion case, so the conversion is evaluated
unconditionally and failure surfaces as an eval error rather than a
process abort.
The same macro also guards string-to-value conversions in
compliance/functions_testlib_interval.cc (4 sites), which were likewise
skipped under NDEBUG; the macro fix restores their evaluation.
Signed-off-by: Fredrik Fornwall
---
googlesql/base/BUILD | 1 +
googlesql/base/status.h | 7 ++++---
googlesql/public/cast.cc | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/googlesql/base/BUILD b/googlesql/base/BUILD
index da713437f..0e3ba456f 100644
--- a/googlesql/base/BUILD
+++ b/googlesql/base/BUILD
@@ -196,6 +196,7 @@ cc_library(
"@com_google_absl//absl/base:core_headers",
"@com_google_absl//absl/base:log_severity",
"@com_google_absl//absl/log",
+ "@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
diff --git a/googlesql/base/status.h b/googlesql/base/status.h
index 16f8282c0..4b163c4af 100644
--- a/googlesql/base/status.h
+++ b/googlesql/base/status.h
@@ -17,8 +17,9 @@
#ifndef THIRD_PARTY_GOOGLESQL_GOOGLESQL_BASE_STATUS_H_
#define THIRD_PARTY_GOOGLESQL_GOOGLESQL_BASE_STATUS_H_
-#include "absl/status/status.h"
-#include "absl/status/statusor.h"
+#include "absl/log/absl_check.h"
+#include "absl/status/status.h"
+#include "absl/status/statusor.h"
// This is better than ABSL_CHECK((val).ok()) because the embedded
// error string gets printed by the ABSL_CHECK_EQ.
@@ -27,7 +28,7 @@
#define GOOGLESQL_DCHECK_OK(val) \
ABSL_DCHECK_EQ(::absl::OkStatus(), ::googlesql::status_internal::AsStatus((val)))
#define GOOGLESQL_QCHECK_OK(val) \
- ABSL_DCHECK_EQ(::absl::OkStatus(), ::googlesql::status_internal::AsStatus((val)))
+ ABSL_QCHECK_EQ(::absl::OkStatus(), ::googlesql::status_internal::AsStatus((val)))
namespace googlesql::status_internal {
diff --git a/googlesql/public/cast.cc b/googlesql/public/cast.cc
index bbae5a60e..85eb52637 100644
--- a/googlesql/public/cast.cc
+++ b/googlesql/public/cast.cc
@@ -1350,7 +1350,7 @@ absl::StatusOr CastContext::CastValue(
}
case FCT(TYPE_DATE, TYPE_DATETIME): {
DatetimeValue datetime;
- GOOGLESQL_QCHECK_OK(
+ GOOGLESQL_RETURN_IF_ERROR(
functions::ConstructDatetime(v.date_value(), TimeValue(), &datetime));
return Value::Datetime(datetime);
}
```
Contributor guide
Research direction
Start with googlesql/base/status.h and googlesql/public/cast.cc, then inspect the absl_check dependency in googlesql/base/BUILD. Run the reported DATE-to-DATETIME query in fastbuild and optimized (-c opt) builds, and check the interval conversion call sites in compliance/functions_testlib_interval.cc. Done means optimized builds preserve the input date and conversion failures remain observable.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100