Copilot commented on code in PR #7871:
URL: https://github.com/apache/texera/pull/7871#discussion_r3837766283
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/sql/SQLSourceOpExecSpec.scala:
##########
@@ -202,7 +219,47 @@ class SQLSourceOpExecSpec extends AnyFlatSpec with
Matchers with MockFactory {
exec.sqlQuery shouldBe Some(BASE + ";")
}
+ it should "skip the sliding window when progressive mode names no batch
column" in {
+ // A progressive descriptor with no batchByColumn is degenerate but
generateSqlQuery is
+ // defensive about it. open() is deliberately not called here: it would
blow up on
+ // `desc.batchByColumn.get`, which is the current (unhelpful) behaviour,
so this test
+ // pins only the query builder. That also means this exact arm of the
line-419 guard
+ // (progressive with no batch column) is defensive: no production caller
reaches
Review Comment:
The comment references a specific production line number ("line-419 guard"),
which will go stale as SQLSourceOpExec.scala changes. Prefer describing the
condition (the progressive-mode guard in generateSqlQuery) without hard-coding
line numbers.
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/sql/SQLSourceOpExecSpec.scala:
##########
@@ -279,13 +336,59 @@ class SQLSourceOpExecSpec extends AnyFlatSpec with
Matchers with MockFactory {
exec.nextQueryAvailable shouldBe false
}
+ it should "offer a next query while an INTEGER batch column is below its
upper bound" in {
+ val conn = boundaryConn(
+ "id",
+ (rs, side) => (rs.getInt(_: Int)).expects(1).returning(if (side ==
"MIN") 0 else 10)
+ )
+ val exec = openedProgressive("id", "auto", "auto", 4L, conn)
+
+ exec.nextQueryAvailable shouldBe true
+ exec.slidingWindow shouldBe " AND id >= 0 AND id < 4"
+
+ // A single-value range (min == max) must still yield exactly one window.
This is the
+ // one input on which the `<=` in hasNextQuery is load-bearing, and unlike
the
+ // exact-multiple case it does not exercise the duplicate-last-window
defect.
+ val singleValueConn =
+ boundaryConn("id", (rs, _) => (rs.getInt(_:
Int)).expects(1).returning(7))
+ val singleValue = openedProgressive("id", "auto", "auto", 4L,
singleValueConn)
+ singleValue.nextQueryAvailable shouldBe true
+ singleValue.slidingWindow shouldBe " AND id >= 7 AND id <= 7"
+ singleValue.nextQueryAvailable shouldBe false
+ }
+
+ it should "refuse to decide the next query for an unsupported batch column
type" in {
+ val exec = new TestSQLSourceOpExec(progressiveJson("name", Option("a"),
Option("z"), 4L))
+ // DEFENSIVE-ONLY arm: intercepting open() is the only way to be holding a
STRING batch
+ // column here, so no workflow can reach this throw. A successful open()
admits only
+ // INTEGER/LONG/TIMESTAMP/DOUBLE -- non-auto boundaries reject all but
LONG/TIMESTAMP,
+ // and auto probes reject BOOLEAN/STRING/ANY -- and all four are handled
above.
+ intercept[IllegalArgumentException](exec.open())
+ intercept[IllegalArgumentException](exec.nextQueryAvailable).getMessage
shouldBe
+ "Unexpected type: string"
Review Comment:
This test depends on open() partially initializing state (batchByAttribute)
before throwing, in order to exercise hasNextQuery's unsupported-type branch.
That makes it brittle to refactors of open() that change when/if
batchByAttribute is set. You can make the test more direct by setting
batchByAttribute explicitly and avoiding the open() call.
--
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]
