Skip to content

[SPARK-58822][SQL][TESTS] Extract a nonEmptyLines helper in LogicalPlanDifferenceSuite - #58048

Closed
uros-b wants to merge 1 commit into
apache:masterfrom
uros-b:helper-planvsplan-nonemptylines
Closed

[SPARK-58822][SQL][TESTS] Extract a nonEmptyLines helper in LogicalPlanDifferenceSuite#58048
uros-b wants to merge 1 commit into
apache:masterfrom
uros-b:helper-planvsplan-nonemptylines

Conversation

@uros-b

@uros-b uros-b commented Aug 17, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Extracts a single private helper in LogicalPlanDifferenceSuite and routes 20 call sites through it:

/** Splits `text` into lines, dropping any empty ones. */
private def nonEmptyLines(text: String): Array[String] =
  text.split("\n").filter(_.nonEmpty)

Why are the changes needed?

The idiom X.split("\n").filter(_.nonEmpty) was repeated 20 times, twice per test, which buries what each test is actually asserting under a parsing incantation. Naming it once reduces the per-test maintenance surface in this suite.

Does this PR introduce any user-facing change?

No. Test-only refactor.

How was this patch tested?

Existing suite. The helper body is the extracted expression verbatim and keeps the Array[String] return type, so every downstream .length, index access, and mkString is unchanged; equivalence holds for empty, single-line, multi-line, and trailing-newline inputs. Scoped to this one file.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtenedor PTAL.

@uros-b
uros-b requested a review from dtenedor August 17, 2026 11:22

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

@uros-b

uros-b commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

Thank you @dongjoon-hyun!

@uros-b uros-b closed this in 67e7317 Aug 18, 2026
uros-b added a commit that referenced this pull request Aug 18, 2026
…nDifferenceSuite

### What changes were proposed in this pull request?

Extracts a single private helper in `LogicalPlanDifferenceSuite` and routes 20 call sites through it:

```scala
/** Splits `text` into lines, dropping any empty ones. */
private def nonEmptyLines(text: String): Array[String] =
  text.split("\n").filter(_.nonEmpty)
```

### Why are the changes needed?

The idiom `X.split("\n").filter(_.nonEmpty)` was repeated 20 times, twice per test, which buries what each test is actually asserting under a parsing incantation. Naming it once reduces the per-test maintenance surface in this suite.

### Does this PR introduce _any_ user-facing change?

No. Test-only refactor.

### How was this patch tested?

Existing suite. The helper body is the extracted expression verbatim and keeps the `Array[String]` return type, so every downstream `.length`, index access, and `mkString` is unchanged; equivalence holds for empty, single-line, multi-line, and trailing-newline inputs. Scoped to this one file.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

Closes #58048 from uros-b/helper-planvsplan-nonemptylines.

Authored-by: Uros <221401595+uros-b@users.noreply.github.com>
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
(cherry picked from commit 67e7317)
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
uros-b added a commit that referenced this pull request Aug 18, 2026
…nDifferenceSuite

### What changes were proposed in this pull request?

Extracts a single private helper in `LogicalPlanDifferenceSuite` and routes 20 call sites through it:

```scala
/** Splits `text` into lines, dropping any empty ones. */
private def nonEmptyLines(text: String): Array[String] =
  text.split("\n").filter(_.nonEmpty)
```

### Why are the changes needed?

The idiom `X.split("\n").filter(_.nonEmpty)` was repeated 20 times, twice per test, which buries what each test is actually asserting under a parsing incantation. Naming it once reduces the per-test maintenance surface in this suite.

### Does this PR introduce _any_ user-facing change?

No. Test-only refactor.

### How was this patch tested?

Existing suite. The helper body is the extracted expression verbatim and keeps the `Array[String]` return type, so every downstream `.length`, index access, and `mkString` is unchanged; equivalence holds for empty, single-line, multi-line, and trailing-newline inputs. Scoped to this one file.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

Closes #58048 from uros-b/helper-planvsplan-nonemptylines.

Authored-by: Uros <221401595+uros-b@users.noreply.github.com>
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
(cherry picked from commit 67e7317)
Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
@uros-b

uros-b commented Aug 18, 2026

Copy link
Copy Markdown
Member Author

Merge Summary:

Posted by merge_spark_pr.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants