Skip to content

Commit adcda16

Browse files
authored
[BUG] Handle null regex captures in interval and regexp_extract_all (#15280)
**JaCoCo sql-plugin line coverage: +25 lines** (8 interval-parsing lines covered by `CsvScanForIntervalSuite`, shim 351; 17 `regexp_extract_all` lines covered by `test_regexp_extract_all_idx_positive`, shim 401) Closes #15275. Closes #15281. ## Root cause [rapidsai/cudf#23123](rapidsai/cudf#23123) changed regex extraction so optional capture groups that do not participate in a match return null instead of an empty string. Two cudf-spark paths relied on the previous representation: - day-time interval parsing treated optional sign and fractional-seconds captures as non-null values; - `regexp_extract_all` removed null list elements, which lost Spark's required empty-string placeholders for unmatched captures and could change list cardinality. Because both failures come from the same cuDF behavior change and independently break premerge, this PR contains the two focused compatibility fixes so the complete Blossom matrix can validate and merge them together. ## Fix Due to the nature of the premerge CI, I have to merge 2 different fixes(though they are caused by the same upstream change) in 1 PR: ### Day-time intervals - Normalize missing sign comparisons to `false` before combining the two signs with XOR. - Normalize a missing fractional-seconds capture to numeric zero before decimal conversion. ### `regexp_extract_all` - Preserve one output list element per regex match. - Convert an unmatched requested capture from null to Spark's empty string. - Preserve empty lists for non-null inputs with no matches and null lists for null inputs. - Restore the input row count when cuDF returns a zero-row list column for an all-no-match batch. The longer-term proposal to move Spark-specific regex result normalization behind the JNI boundary is tracked in [NVIDIA/cudf-spark-jni#4821](NVIDIA/cudf-spark-jni#4821). This PR keeps the current ownership boundaries and fixes the two affected consumers without adding a temporary compatibility switch. ## Validation - Scala 2.12 / Spark 3.5.1 `CsvScanForIntervalSuite`: `Tests: succeeded 6, failed 0, canceled 1, ignored 0, pending 0`; reactor `BUILD SUCCESS`. - Scala 2.13 / Spark 4.0.1 production and integration-test build: reactor `BUILD SUCCESS`. - Spark 4.0.1 `regexp_test.py::test_regexp_extract_all_idx_positive`: `3 passed, 39627 deselected`. - The same regex IT with forced OOM injection: `3 passed, 39627 deselected`. - JaCoCo fix-line intersection: 8 of 16 added interval lines plus 17 of 30 added regex lines covered, for 25 unique added production lines. - `scripts/check-shim-coverage.sh`: passed. ## Performance impact The interval change adds null normalization only in the specialized string-to-day-time-interval path. The regex change removes the previous max-list-width expansion and reconstructs the existing variable-length list column in place; the focused microbenchmark measured 0.538 s before versus 0.464 s after (about 13.7% faster). No new regex match pass is introduced. Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [ ] Added or modified tests to cover new code paths - [x] Covered by existing tests (`CsvScanForIntervalSuite` and `regexp_test.py::test_regexp_extract_all_idx_positive`, including forced OOM injection.) - [ ] Not required Performance - [x] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [ ] Not required --------- Signed-off-by: Allen Xu <allxu@nvidia.com>
1 parent 306a657 commit adcda16

2 files changed

Lines changed: 46 additions & 46 deletions

File tree

sql-plugin/src/main/scala/com/nvidia/spark/rapids/shims/GpuIntervalUtilsBase.scala

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,14 @@ trait GpuIntervalUtilsBase {
286286
firstSignInTable: ColumnVector, secondSignInTable: ColumnVector): ColumnVector = {
287287
val negatives = withResource(Scalar.fromString("-")) { negScalar =>
288288
withResource(Seq(firstSignInTable, secondSignInTable).safeMap(negScalar.equalTo)) {
289-
case Seq(neg1, neg2) => neg1.bitXor(neg2)
289+
signMatches =>
290+
// cuDF returns null for optional capture groups that did not participate in the match.
291+
// A missing sign is positive, so normalize those null comparisons to false before XOR.
292+
withResource(Scalar.fromBool(false)) { falseScalar =>
293+
withResource(signMatches.safeMap(_.replaceNulls(falseScalar))) {
294+
case Seq(neg1, neg2) => neg1.bitXor(neg2)
295+
}
296+
}
290297
}
291298
}
292299

@@ -307,8 +314,14 @@ trait GpuIntervalUtilsBase {
307314
protected def getMicrosFromDecimal(sign: ColumnVector, decimal: ColumnVector): ColumnVector = {
308315
val decimalType64_6 = DType.create(DType.DTypeEnum.DECIMAL64, -6)
309316
val timesMillion = withResource(Scalar.fromLong(1000000L)) { million =>
310-
withResource(decimal.castTo(decimalType64_6)) {
311-
_.mul(million)
317+
// An absent optional fractional-seconds capture is null. It represents zero micros rather
318+
// than a failed parse, so normalize it before the decimal conversion.
319+
withResource(Scalar.fromString("0")) { zero =>
320+
withResource(decimal.replaceNulls(zero)) { normalizedDecimal =>
321+
withResource(normalizedDecimal.castTo(decimalType64_6)) {
322+
_.mul(million)
323+
}
324+
}
312325
}
313326
}
314327
val timesMillionLongs = withResource(timesMillion) {

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/stringFunctions.scala

Lines changed: 30 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import com.nvidia.spark.rapids.jni.RegexRewriteUtils
3737
import com.nvidia.spark.rapids.shims.{NullIntolerantShim, ShimExpression, SparkShimImpl}
3838

3939
import org.apache.spark.sql.catalyst.expressions._
40+
import org.apache.spark.sql.catalyst.util.GenericArrayData
4041
import org.apache.spark.sql.errors.ConvUtils
4142
import org.apache.spark.sql.rapids.catalyst.expressions._
4243
import org.apache.spark.sql.types._
@@ -1753,58 +1754,44 @@ case class GpuRegExpExtractAll(
17531754
EnumSet.of(RegexFlag.EXT_NEWLINE), CaptureGroups.NON_CAPTURE)
17541755
str.getBase.extractAllRecord(prog, 0)
17551756
case _ =>
1756-
// Extract matches corresponding to idx. cuDF's extract_all_record does not support
1757-
// group idx, so we must manually extract the relevant matches. Example:
1758-
// Given the pattern (\d+)-(\d+) and idx=1
1759-
//
1760-
// | Input | Java | cuDF |
1761-
// |-----------------|-----------------|--------------------------------|
1762-
// | '1-2, 3-4, 5-6' | ['1', '3', '5'] | ['1', '2', '3', '4', '5', '6'] |
1763-
//
1764-
// Since idx=1 and the pattern has 2 capture groups, we take the 1st element and every
1765-
// 2nd element afterwards from the cuDF list
1766-
17671757
val rowCount = str.getRowCount
17681758
val prog = new RegexProgram(cudfRegexPattern, EnumSet.of(RegexFlag.EXT_NEWLINE))
17691759

1770-
val extractedWithNulls = withResource(
1771-
// Now the index is always 1 because we have transpiled all the capture groups to the
1772-
// single group that we care about, so we just have to handle the idx = 1 case here
1773-
str.getBase.extractAllRecord(prog, 1)) { allExtracted =>
1774-
withResource(allExtracted.countElements) { listSizes =>
1775-
withResource(listSizes.max) { maxSize =>
1776-
val maxSizeInt = maxSize.getInt
1777-
val stringCols = Range(0, maxSizeInt, 1).safeMap {
1778-
i =>
1779-
allExtracted.extractListElement(i)
1780-
}
1781-
withResource(stringCols) { _ =>
1782-
ColumnVector.makeList(rowCount, DType.STRING, stringCols: _*)
1783-
}
1760+
// The transpiler leaves only the requested group as a capture group, so cuDF already
1761+
// returns one list element per regex match. Align the remaining result semantics with
1762+
// Spark: an unmatched capture is an empty string, no matches is an empty list, and a
1763+
// null input is a null list.
1764+
withResource(str.getBase.extractAllRecord(prog, 1)) { extracted =>
1765+
val noMatchesAsEmptyLists = withResource(GpuScalar.from(
1766+
new GenericArrayData(Array.empty[Any]), dataType)) { emptyStringList =>
1767+
// cuDF returns a zero-row list column when the entire input has no matches.
1768+
// Restore the input row count before applying Spark's null-input semantics.
1769+
if (extracted.getRowCount == 0) {
1770+
ColumnVector.fromScalar(emptyStringList, rowCount.toInt)
1771+
} else {
1772+
val capturesWithEmptyStrings = withResource(extracted.getChildColumnView(0)) {
1773+
captures =>
1774+
withResource(Scalar.fromString("")) { emptyString =>
1775+
withResource(captures.replaceNulls(emptyString)) { normalizedCaptures =>
1776+
withResource(extracted.replaceListChild(normalizedCaptures)) {
1777+
_.copyToColumnVector()
1778+
}
1779+
}
1780+
}
17841781
}
1785-
}
1786-
}
1787-
// Filter out null values in the lists
1788-
val extractedStrings = withResource(extractedWithNulls) { _ =>
1789-
val booleanMask = withResource(extractedWithNulls.getListOffsetsView) { offsetsCol =>
1790-
withResource(extractedWithNulls.getChildColumnView(0)) { stringCol =>
1791-
withResource(stringCol.isNotNull) { isNotNull =>
1792-
isNotNull.makeListFromOffsets(rowCount, offsetsCol)
1782+
withResource(capturesWithEmptyStrings) { normalized =>
1783+
withResource(normalized.isNull) { noMatchesOrNullInput =>
1784+
noMatchesOrNullInput.ifElse(emptyStringList, normalized)
1785+
}
17931786
}
17941787
}
17951788
}
1796-
withResource(booleanMask) {
1797-
extractedWithNulls.applyBooleanMask
1798-
}
1799-
}
1800-
1801-
// If input is null, output should also be null
1802-
withResource(extractedStrings) { s =>
1803-
withResource(GpuScalar.from(null, DataTypes.createArrayType(DataTypes.StringType))) {
1804-
nullStringList =>
1789+
withResource(noMatchesAsEmptyLists) { normalized =>
1790+
withResource(GpuScalar.from(null, dataType)) { nullStringList =>
18051791
withResource(str.getBase.isNull) { isInputNull =>
1806-
isInputNull.ifElse(nullStringList, s)
1792+
isInputNull.ifElse(nullStringList, normalized)
18071793
}
1794+
}
18081795
}
18091796
}
18101797
}

0 commit comments

Comments
 (0)