1: Field/parameter names are not case insensitive #61
Conversation
* introduced classes `ColumnReference` and `ColumnName` to represent column and parameter names and handle them properly in SQL manner * Reworked `Params` class hierarchy to work with the new classes and for better structure * Introduced `SqlItem` trait shared between all classes providing `SQLEntry` * `DBTable`, `DBFunction` and `DBQuerySupport` now utilize the streamlined `Params` structure
benedeki
requested review from
lsulak,
miroslavpojer,
salamonpavel and
tmikula-dev
as code owners
WalkthroughSeparates ordered and named SQL parameters, introduces ColumnReference/SqlItem for normalized/quoted identifiers, migrates internal collections from List to Vector, renames metadata/lookup APIs to "columnName", adds tests and SQL fixtures for strange column names, and loads those fixtures in CI DB setup. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (1)
53-53: Consider returningOption[String]fromunapplyfor idiomatic pattern matching.The
unapplymethod returnsStringdirectly instead ofOption[String]. While this works in Scala 3 and some Scala 2 contexts, the conventional pattern for extractors is:def unapply(columnName: ColumnName): Option[String] = Some(columnName.enteredName)This ensures compatibility with all pattern matching contexts.
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (2)
31-35: Consider adding scaladoc to abstract methods.The abstract methods lack documentation. While semantics are intuitive, documenting expected behavior (e.g., whether
apply(position)is 0-based) would improve API clarity.
167-169: Consider documenting thatapply(position)uses 0-based indexing.The
apply(position: Int)method uses 0-based vector indexing, while the internally storedColumnIndexis 1-based (for SQL). This is correct but potentially confusing for API users expecting SQL-style 1-based positions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/build.yml(1 hunks).github/workflows/jacoco_report.yml(1 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala(8 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala(5 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala(2 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala(7 hunks)balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala(1 hunks)balta/src/main/scala/za/co/absa/db/mag/core/SqlItem.scala(1 hunks)balta/src/test/resources/db/postgres/10_testing.strange_columns.sql(1 hunks)balta/src/test/resources/db/postgres/11_testing.strange_columns_data.sql(1 hunks)balta/src/test/scala/za/co/absa/db/balta/DBTestSuiteUnitTests.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala(2 hunks)balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
sql(39-47)
balta/src/main/scala/za/co/absa/db/mag/core/SqlItem.scala (1)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (1)
sqlEntry(56-56)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (5)
apply(123-123)apply(125-125)apply(134-134)apply(167-167)apply(173-173)
balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala (1)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (7)
ColumnReference(33-68)sqlEntry(56-56)hashCode(30-30)unapply(53-53)unapply(72-72)ColumnName(21-31)ColumnName(70-73)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (3)
balta/src/main/scala/za/co/absa/db/balta/implicits/MapImplicits.scala (1)
getOrThrow(28-30)balta/src/main/scala/za/co/absa/db/balta/implicits/OptionImplicits.scala (1)
getOrThrow(27-29)balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (3)
apply(38-47)apply(49-51)apply(71-71)
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (3)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (8)
Params(29-36)Params(37-175)OrderedParams(141-170)OrderedParams(172-174)values(127-127)values(169-169)paramNames(130-130)items(128-128)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (2)
sqlEntry(24-24)equalityOperator(25-25)balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
runQuery(26-47)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (1)
QueryParamType(32-92)balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (18)
Params(29-36)Params(37-175)NamedParams(96-131)NamedParams(133-135)OrderedParams(141-170)OrderedParams(172-174)values(127-127)values(169-169)items(128-128)add(51-53)add(75-77)add(105-109)add(149-153)apply(123-123)apply(125-125)apply(134-134)apply(167-167)apply(173-173)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
sqlEntry(24-24)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (2)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (6)
ColumnName(21-31)ColumnName(70-73)ColumnReference(33-68)apply(38-47)apply(49-51)apply(71-71)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (16)
QueryParamType(32-92)NULL(86-86)toQueryParamValue(35-35)toQueryParamValue(39-39)toQueryParamValue(43-43)toQueryParamValue(47-47)toQueryParamValue(51-51)toQueryParamValue(55-55)toQueryParamValue(59-59)toQueryParamValue(63-63)toQueryParamValue(67-67)toQueryParamValue(71-71)toQueryParamValue(75-75)toQueryParamValue(79-79)toQueryParamValue(83-83)toQueryParamValue(89-89)
balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala (2)
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (7)
delete(159-161)delete(163-165)DBTable(29-257)where(80-82)where(93-95)where(105-107)where(118-120)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (1)
NULL(86-86)
🔇 Additional comments (24)
.github/workflows/build.yml (1)
74-75: LGTM!The additional database setup steps correctly load the schema and test data for the new strange column name tests, following the established pattern.
balta/src/test/resources/db/postgres/11_testing.strange_columns_data.sql (1)
17-19: LGTM!The INSERT statement correctly populates test data with unconventional column identifiers (mixed case, spaces, special characters, Unicode, starting with digit), which aligns with the PR objective to test case-insensitive handling and quoting of column names.
balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala (2)
95-95: LGTM!The update to
Params.add("text_field", NULL)correctly uses the new explicit-value API, replacing the deprecatedaddNullmethod.
176-238: Excellent test coverage for unconventional column identifiers.This test comprehensively validates the PR objectives by exercising:
- Inserts with both unquoted and explicitly quoted column names
- Query filtering across various special identifier formats (mixed case, spaces, hyphens, accented characters, digit-prefixed)
- Proper cleanup
The test ensures that the framework correctly handles SQL-standard identifier quoting and case sensitivity rules.
.github/workflows/jacoco_report.yml (1)
51-52: LGTM!Consistent with the build workflow, these additions ensure the code coverage job has access to the strange column test data.
balta/src/main/scala/za/co/absa/db/mag/core/SqlItem.scala (1)
19-21: LGTM!Clean trait providing a lightweight contract for types that produce SQL fragment representations. This supports the new column reference system introduced in the PR.
balta/src/test/resources/db/postgres/10_testing.strange_columns.sql (1)
17-33: LGTM!The table schema provides comprehensive coverage of SQL identifier edge cases, including mixed case, spaces, special characters, Unicode, and digit-prefixed names. This properly supports testing the PR's objectives around case-insensitive handling and automatic quoting of non-standard identifiers.
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
26-26: All runQuery call sites properly updated to Vector.Verified that all callers pass
Vector[QueryParamValue]torunQuery:
- DBFunction.scala:96 uses
orderedParams.values ++ namedParams.values(both return Vector)- DBTable.scala:46, 223, 231, 238 all receive
Vector[QueryParamValue]parametersThe parameter type change from
List[QueryParamValue]toVector[QueryParamValue]is complete across all implementations.balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (3)
39-46: LGTM! Clean pattern matching for INSERT column handling.The implementation correctly differentiates between
NamedParams(which generates explicit column names) andOrderedParams(which relies on positional VALUES only). UsingcolumnName.sqlEntryensures proper quoting for column names with special characters.
251-256: LGTM! WHERE clause now uses SQL-safe column names.The use of
columnName.sqlEntryensures column names with special characters or mixed case are properly quoted in the generated WHERE clause.
217-224: LGTM! Vector is appropriate for parameter collections.The change from
ListtoVectoraligns withDBQuerySupport.runQuerysignature and provides better performance characteristics for the indexed iteration patterns used during parameter binding.balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (2)
152-164: LGTM! Clean rename from FieldNames to ColumnNames.The type alias and method signature updates are consistent with the broader naming convention changes in the PR.
42-51: Reject case-insensitive normalization; column names should remain case-sensitive per PR design.The PR objective explicitly states "Field/parameter names are NOT case insensitive." The current implementation correctly preserves column name case as returned by the database metadata. Normalizing unquoted labels to lowercase would contradict this design decision and break databases that return uppercase or mixed-case column names (e.g., Oracle). The quoted vs. unquoted handling in the current code (lines 43-50) properly respects SQL semantics where quoted identifiers retain their case while unquoted identifiers follow database-specific folding rules.
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (1)
21-31: LGTM! Equality based onsqlEntrycorrectly implements case-insensitive semantics.The
equalsandhashCodeimplementations ensure that different representations of the same SQL identifier (e.g.,aband"ab") are treated as equal, which aligns with the PR objective of SQL-standard case insensitivity.balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (3)
39-47: LGTM! SQL generation correctly separates positional and named parameters.The implementation properly:
- Generates positional parameter placeholders from
orderedParams.values- Generates named parameter assignments using
columnName.sqlEntry := value.sqlEntry- Concatenates them in the correct order (positional first, then named)
94-97: LGTM! Parameter binding order matches SQL placeholder order.The combined values
orderedParams.values ++ namedParams.valuescorrectly align with the SQL placeholder order generated insql().
146-152: LGTM! Convenient factory methods for common parameter patterns.The overloads cleanly support both named-only and positional-only parameter initialization scenarios.
balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala (1)
68-72: Verify thatsqlEntryproperly wraps quoted identifiers.The SQL standard requires quoted identifiers to be wrapped in delimiters with internal quotes doubled. An identifier containing a quote should produce
"a""b"(in SQL), which in Scala string form would be"\"a\"\"b\"", not"a\"\"b"as currently expected by the test.If the implementation produces unquoted output like
a""b, the resulting SQL would be malformed—interpreted as multiple tokens rather than a single identifier. Either the implementation or test expectations need correction to properly quote the identifier.balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (6)
105-109: LGTM — immutable update pattern is correct.The approach of creating a new
NamedParamsinstance with updated map is clean and thread-safe. TheListMappreserves insertion order as needed.
123-123: Lookup relies on ColumnName equality — verify consistency.The lookup creates a new
ColumnName(paramName)and uses it as a map key. This will only work correctly ifColumnNameequality is case-insensitive for standard identifiers. Ifadd("Param1", v)stores aColumnNameQuotedbutapply("param1")looks up withColumnNameSimple, the lookup will fail withNoSuchElementException.Consider providing a safer accessor that returns
Option[QueryParamValue]or validates the key exists.
149-153: LGTM — parameter indexing is consistent.The
ColumnIndexuses 1-based indexing (SQL convention) while vector storage uses 0-based indexing. This is correct for SQL parameter binding.
62-65: Good deprecation practice with clear migration path.The deprecation annotation includes version info and guidance to use
add(paramName, QueryParamType.NULL)instead. Clean approach for phasing out the dedicated null methods.
133-135: LGTM — companion object factories.Clean factory methods enabling
NamedParams()andOrderedParams()construction without exposing thenewkeyword externally.Also applies to: 172-174
39-40: Verify case-insensitive lookup behavior with ColumnName keys in code review.The
NamedParamsMapusesColumnNameas keys, which could potentially cause case-sensitivity issues. Ensure that theColumnNametype andColumnReference.apply()method properly handle case normalization. Specifically, verify thatColumnName("param1")andColumnName("PARAM1")resolve to equivalent keys when representing unquoted SQL identifiers, particularly if Issue #1 establishes a requirement for SQL case-insensitivity alignment.
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
Outdated
Show resolved
Hide resolved
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
Outdated
Show resolved
Hide resolved
balta/src/test/scala/za/co/absa/db/balta/DBTestSuiteUnitTests.scala
Outdated
Show resolved
Hide resolved
balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala
Show resolved
Hide resolved
JaCoCo 'balta' module code coverage report - scala 2.12.18
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResult.scala(2 hunks)balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala(1 hunks)balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala
🧰 Additional context used
🧬 Code graph analysis (2)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (3)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (5)
apply(123-123)apply(125-125)apply(134-134)apply(167-167)apply(173-173)balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (3)
apply(142-144)apply(146-148)apply(150-152)balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (3)
apply(59-59)apply(65-65)apply(157-160)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResult.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (3)
QueryResultRow(36-148)QueryResultRow(150-194)fieldNamesFromMetadata(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Scala 2.11.12
- GitHub Check: Scala 2.12.18
🔇 Additional comments (5)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResult.scala (1)
19-35: ColumnNames import and implicit type alignment look correctThe new
ColumnNamesimport and the updated implicitfieldNames: ColumnNamesare consistent withQueryResultRow.fieldNamesFromMetadataand keep behavior unchanged. No functional or API concerns here from this refactor.balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (4)
38-39: Helper methods are correctly implemented.The
quoteandescapeQuotefunctions properly handle SQL identifier quoting: internal quotes are doubled, then the result is wrapped in double quotes (e.g.,a"b→"a""b").
57-65: Case classes correctly model the domain.The three case classes properly represent simple column names, exact (quoted) names, and index-based references with appropriate
sqlEntrygeneration.
51-56: LGTM: Companion object methods follow Scala conventions.The factory methods and extractors provide clean APIs for constructing and pattern-matching column references.
Also applies to: 68-71
24-30:equalsandhashCodeimplementation is correct.Comparing
sqlEntryvalues provides the right semantics for case-insensitive equality once the factory method properly normalizes unquoted identifiers (see comment on lines 34-35).
| private val regularColumnNamePattern = "^([a-z_][a-z0-9_]*)$".r | ||
| private val quotedRegularColumnNamePattern = "^\"([a-z_][a-z0-9_]*)\"$".r |
There was a problem hiding this comment.
Critical: Regex patterns break case-insensitive identifier handling.
Two issues prevent proper case-insensitive comparison:
-
Line 34:
regularColumnNamePatternonly matches lowercase identifiers. Uppercase unquoted identifiers likeABCfall through to line 48 and get quoted as"ABC", making them unequal toabc. Per SQL semantics and the PR objective (Issue Field/parameter names are not case insensitive #1), unquoted identifiers should be case-insensitive. -
Line 35: Missing
*quantifier causesquotedRegularColumnNamePatternto only match 2-character identifiers.
Apply this diff:
- private val regularColumnNamePattern = "^([a-z_][a-z0-9_]*)$".r
- private val quotedRegularColumnNamePattern = "^\"([a-z_][a-z0-9_]*)\"$".r
+ private val regularColumnNamePattern = "^([a-zA-Z_][a-zA-Z0-9_]*)$".r
+ private val quotedRegularColumnNamePattern = "^\"([a-z_][a-z0-9_]*)\"$".rThen normalize in the factory method (line 44):
- case regularColumnNamePattern(columnName) => ColumnNameSimple(columnName)
+ case regularColumnNamePattern(columnName) => ColumnNameSimple(columnName.toLowerCase)This ensures abc, ABC, and AbC all map to the same sqlEntry value for case-insensitive equality.
🤖 Prompt for AI Agents
In balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala around
lines 34-35, the two regexes are incorrect: change regularColumnNamePattern to
accept letters in both cases (e.g. use [A-Za-z_] for the first char and
[A-Za-z0-9_]* for the rest) and fix quotedRegularColumnNamePattern to use the *
quantifier so it matches identifiers of any length; then in the factory method
around line 44, normalize matched identifier names to a consistent case (e.g.
toLowerCase) when creating the sqlEntry so unquoted identifiers like abc, ABC
and AbC produce the same sqlEntry for case-insensitive equality.
There was a problem hiding this comment.
Unquoted mixed case identifiers are auto-quoted.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the ColumnReferenceUnitTests to see what's tested for mixed-case identifiers
cat balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala
Length of output: 4574
@benedeki, I understand the current implementation auto-quotes mixed-case identifiers, but this approach contradicts the PR objective from Issue #1 and SQL standard semantics:
SQL Standard Behavior:
- Unquoted identifiers are case-insensitive:
abc,ABC, andAbCall refer to the same column - Quoted identifiers are case-sensitive:
"abc"≠"ABC"
Current Implementation:
abc→abc(unquoted)ABC→"ABC"(quoted, now case-sensitive)AbC→"AbC"(quoted, now case-sensitive)
This means abc ≠ ABC ≠ AbC, violating Issue #1's objective: "Ensure parameter/field name handling is case-insensitive to align framework behavior with SQL semantics."
Example Problem:
val params = NamedParams().add("UserId", 123)
// Creates "UserId" (quoted, case-sensitive)
// Won't match a column named userid or USERIDThe test on Line 100 (assert(n1 != n2, "Expected inequality for different sqlEntry")) encodes this behavior but contradicts Issue #1.
Question: Is the intent to preserve mixed-case identifiers as case-sensitive (current implementation), or to make all unquoted identifiers case-insensitive per SQL standard (Issue #1 objective)? If the latter, unquoted identifiers should be normalized (e.g., lowercased) regardless of how they're entered.
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
Outdated
Show resolved
Hide resolved
…ain other non-standard characters * unified naming - columnName(s) is used all around
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala(6 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/QueryResult.scala(2 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala(6 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala(7 hunks)balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala(1 hunks)balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala(2 hunks)balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala(3 hunks)balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala(1 hunks)balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala
- balta/src/main/scala/za/co/absa/db/balta/classes/QueryResult.scala
🧰 Additional context used
🧬 Code graph analysis (7)
balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (5)
QueryResultRow(35-139)QueryResultRow(141-185)columnNamesFromMetadata(153-155)getOffsetDateTime(109-109)getOffsetDateTime(110-110)
balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (3)
QueryResultRow(35-139)QueryResultRow(141-185)columnNamesFromMetadata(153-155)
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (2)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (12)
Params(29-36)Params(37-175)OrderedParams(141-170)OrderedParams(172-174)values(127-127)values(169-169)paramNames(130-130)add(51-53)add(75-77)add(105-109)add(149-153)items(128-128)balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
runQuery(26-47)
balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala (6)
balta/src/main/scala/za/co/absa/db/mag/naming/implementations/ExplicitNamingRequired.scala (1)
stringPerConvention(33-36)balta/src/main/scala/za/co/absa/db/mag/naming/implementations/MapBasedNaming.scala (1)
stringPerConvention(35-37)balta/src/main/scala/za/co/absa/db/mag/naming/implementations/AsIsNaming.scala (1)
stringPerConvention(34-36)balta/src/main/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNaming.scala (1)
stringPerConvention(49-51)balta/src/main/scala/za/co/absa/db/balta/implicits/OptionImplicits.scala (1)
value(37-37)balta/src/main/scala/za/co/absa/db/balta/implicits/MapImplicits.scala (1)
getOrThrow(28-30)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (2)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (9)
core(40-40)core(41-41)core(42-45)ColumnName(21-32)ColumnName(77-80)ColumnReference(34-75)apply(47-66)apply(68-70)apply(78-78)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (16)
QueryParamType(32-92)NULL(86-86)toQueryParamValue(35-35)toQueryParamValue(39-39)toQueryParamValue(43-43)toQueryParamValue(47-47)toQueryParamValue(51-51)toQueryParamValue(55-55)toQueryParamValue(59-59)toQueryParamValue(63-63)toQueryParamValue(67-67)toQueryParamValue(71-71)toQueryParamValue(75-75)toQueryParamValue(79-79)toQueryParamValue(83-83)toQueryParamValue(89-89)
balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala (1)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (8)
core(40-40)core(41-41)core(42-45)ColumnReference(34-75)hashCode(31-31)ColumnName(21-32)ColumnName(77-80)unapply(79-79)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (2)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (8)
core(40-40)core(41-41)core(42-45)ColumnName(21-32)ColumnName(77-80)apply(47-66)apply(68-70)apply(78-78)balta/src/main/scala/za/co/absa/db/balta/implicits/MapImplicits.scala (1)
getOrThrow(28-30)
🪛 GitHub Actions: Build
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
[error] 21-21: method unapply is defined twice; conflicting symbols both originated in this file
[error] 22-22: Compilation failed: error near 'case class ColumnName private(enteredName: String,'
🔇 Additional comments (12)
balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala (1)
28-32: LGTM!The name-based overloads follow a clean delegation pattern, routing through
row.columnNumber(columnName)which now properly handles case-insensitive column lookups via the newColumnNameinfrastructure.balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala (1)
75-89: LGTM!The refactor consistently updates terminology from
columnLabeltocolumnNamethroughout. The TODO comment properly tracks the TypeTag cleanup task via issue #64. The error message at line 87 correctly reflects the new column name terminology.balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala (1)
32-33: LGTM!Test correctly updated to reflect the API rename from
fieldNamesFromMetadatatocolumnNamesFromMetadata, also fixing the typo in the test name ("Metada" → "Metadata").balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala (1)
32-35: LGTM!Tests correctly updated to reflect the API rename:
- Method call updated to
columnNamesFromMetadata- Typo fixed:
expecedResult→expectedResult- Variable renamed:
resultOfColumnLabel→resultOfColumnNamefor consistencyAlso applies to: 54-54, 195-197
balta/src/test/scala/za/co/absa/db/mag/core/ColumnReferenceUnitTests.scala (1)
21-166: Comprehensive test coverage for ColumnReference API.The test suite thoroughly validates the new
ColumnReferencefunctionality including:
- Regular and quoted name handling
- Case-insensitivity via
sqlEntry-based equality- Non-standard characters (spaces, dashes, non-ASCII, quotes)
- Quote escaping and unescaping
- Index-based column references
This aligns well with the PR objectives to handle column names per SQL standard.
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (3)
38-46: LGTM!The pattern matching on
Paramstype correctly handles bothNamedParams(generates column list fromparamNames) andOrderedParams(empty column list for positional insertion). The use of_.sqlEntryensures proper SQL quoting for non-standard column names.
217-241: LGTM!The parameter type change from
List[QueryParamValue]toVector[QueryParamValue]aligns with the broader refactor acrossDBQuerySupport.runQueryand theParamshierarchy.
251-256: LGTM!The refactored
paramsToWhereConditioncorrectly usescolumnName.sqlEntryto generate SQL-safe column references, ensuring proper quoting for columns with non-standard characters. This directly addresses PR objective #2 (escaping column/parameter names).balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (2)
21-23: LGTM!The new imports and type aliases cleanly integrate
ColumnNameandColumnReferencefrom the mag.core package. TheParamsListtype alias provides a clear contract for the items representation.Also applies to: 39-40
62-65: Deprecation annotations are well-structured.Good use of
@deprecatedwith clear migration guidance ("Use add(paramName, NULL)") and version info ("balta 0.3.0").Also applies to: 86-89, 118-121, 162-165
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (2)
40-43: LGTM on the column lookup logic.Using
ColumnName(columnName).quoteLesscorrectly normalizes user input for case-insensitive lookups, aligning with the PR objective from Issue #1.
143-143: Rename fromFieldNamestoColumnNamesis appropriate.The new name better reflects SQL terminology and aligns with the
ColumnName/ColumnReferenceabstractions introduced in this PR.
| def apply(paramName: String): QueryParamValue = namedParams(ColumnName(paramName)) | ||
|
|
||
| override def keys: Option[List[String]] = Some(items.keys.toList) | ||
| override def apply(position: Int): QueryParamValue = values(position) |
There was a problem hiding this comment.
Critical: Same off-by-one error in NamedParams.apply(position: Int).
If position is 1-based (consistent with SQL conventions and OrderedParams which uses 1-based ColumnReference indices), then values(position) should be values(position - 1).
🔎 Proposed fix
- override def apply(position: Int): QueryParamValue = values(position)
+ override def apply(position: Int): QueryParamValue = values(position - 1)Alternatively, clarify in the Scaladoc whether position is 0-based or 1-based to ensure consistent usage across the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override def apply(position: Int): QueryParamValue = values(position) | |
| override def apply(position: Int): QueryParamValue = values(position - 1) |
🤖 Prompt for AI Agents
In balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala around
line 125, NamedParams.apply currently uses values(position) which causes a 1-off
error when callers pass SQL-style 1-based positions; change the implementation
to index values(position - 1) and add a guard to validate position is between 1
and values.length (throw IndexOutOfBoundsException or similar) and update the
Scaladoc for NamedParams.apply to state that position is 1-based so callers and
OrderedParams remain consistent.
| override def apply(position: Int): QueryParamValue = items(position)._2 | ||
| override def size: Int = items.size | ||
| override def values: Vector[QueryParamValue] = items.map(_._2) |
There was a problem hiding this comment.
Critical: Off-by-one error in apply(position: Int).
ColumnReference indices start at 1 (line 150: ColumnReference(size + 1)), but items is a 0-indexed Vector. Calling apply(1) accesses items(1) (second element) instead of items(0) (first element), causing IndexOutOfBoundsException for the first parameter.
🔎 Proposed fix
- override def apply(position: Int): QueryParamValue = items(position)._2
+ override def apply(position: Int): QueryParamValue = items(position - 1)._2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override def apply(position: Int): QueryParamValue = items(position)._2 | |
| override def size: Int = items.size | |
| override def values: Vector[QueryParamValue] = items.map(_._2) | |
| override def apply(position: Int): QueryParamValue = items(position - 1)._2 | |
| override def size: Int = items.size | |
| override def values: Vector[QueryParamValue] = items.map(_._2) |
🤖 Prompt for AI Agents
In balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala around
lines 167 to 169, apply(position: Int) currently uses 0-based indexing against
the 1-based ColumnReference position, causing off-by-one and potential
IndexOutOfBounds; change the implementation to use items(position - 1)._2 and
add a bounds check (e.g., require(position >= 1 && position <= size, s"Invalid
parameter position: $position")) so callers using 1-based positions get the
correct element and invalid positions produce a clear error.
| def columnNamesFromMetadata(metaData: ResultSetMetaData): ColumnNames = { | ||
| Range.inclusive(1, metaData.getColumnCount).map(i => metaData.getColumnName(i) -> i).toMap | ||
| } |
There was a problem hiding this comment.
Major: Column name lookup will fail due to inconsistent normalization.
columnNumber (lines 40-42) normalizes the input via ColumnName(columnName).quoteLess (e.g., "MyColumn" → "mycolumn"), but columnNamesFromMetadata stores raw metadata names as keys without normalization.
This causes lookups to fail for mixed-case columns:
- Metadata returns
"MyColumn"→ stored as key"MyColumn" - User queries
"MyColumn"→ lookup key is"mycolumn"→NoSuchElementException
🔎 Proposed fix: Normalize keys when building the map
def columnNamesFromMetadata(metaData: ResultSetMetaData): ColumnNames = {
- Range.inclusive(1, metaData.getColumnCount).map(i => metaData.getColumnName(i) -> i).toMap
+ Range.inclusive(1, metaData.getColumnCount).map { i =>
+ val columnName = ColumnName(metaData.getColumnName(i))
+ columnName.quoteLess -> i
+ }.toMap
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def columnNamesFromMetadata(metaData: ResultSetMetaData): ColumnNames = { | |
| Range.inclusive(1, metaData.getColumnCount).map(i => metaData.getColumnName(i) -> i).toMap | |
| } | |
| def columnNamesFromMetadata(metaData: ResultSetMetaData): ColumnNames = { | |
| Range.inclusive(1, metaData.getColumnCount).map { i => | |
| val columnName = ColumnName(metaData.getColumnName(i)) | |
| columnName.quoteLess -> i | |
| }.toMap | |
| } |
There was a problem hiding this comment.
The normalization of the keys might make the map collapse more keys into one.
The logic here is, that if the user uses the same string for the query and the result it behaves the same way. If user wants a mixed case in column name, they have to enter it quoted.
There was a problem hiding this comment.
@benedeki Thank you for the clarification! I understand now—storing the raw metadata names preserves the exact column identifiers as returned by the database and avoids potential key collisions in edge cases where a database might have columns that differ only in case.
Your design makes sense: users need to access result columns using the same naming convention they used in the query (quoted for mixed-case, unquoted for standard identifiers). This maintains consistency between query definition and result access.
I appreciate you explaining the rationale—it's a thoughtful approach to handling SQL identifier semantics!
✏️ Learnings added
Learnt from: benedeki
Repo: AbsaOSS/balta PR: 61
File: balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala:153-155
Timestamp: 2025-12-31T11:12:57.166Z
Learning: In QueryResultRow, column names from metadata are stored as-is without normalization to avoid key collisions. Users must access columns using the same naming convention as in their queries: quoted identifiers (e.g., `"MyColumn"`) for mixed-case columns, unquoted for standard lowercase identifiers. This design preserves exact database column names and maintains query-to-result consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
Outdated
Show resolved
Hide resolved
|
Lol, by the bot, |
lsulak
left a comment
lsulak
left a comment
There was a problem hiding this comment.
Good job overall, I like the tests and column label -> name changes as well!
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scalabalta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala
🧰 Additional context used
🧬 Code graph analysis (2)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
sql(39-47)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (18)
Params(29-36)Params(37-175)NamedParams(96-131)NamedParams(133-135)OrderedParams(141-170)OrderedParams(172-174)values(127-127)values(169-169)items(128-128)add(51-53)add(75-77)add(105-109)add(149-153)apply(123-123)apply(125-125)apply(134-134)apply(167-167)apply(173-173)
🪛 GitHub Actions: Build
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala
[error] 163-163: identifier expected but ')' found.
[error] 200-200: ':' expected but '}' found.
🔇 Additional comments (7)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
20-25: LGTM! Clean integration with SqlItem trait.The refactor to extend
SqlItemand provide a defaultsqlEntryimplementation is well-structured. The"?"placeholder is appropriate for standard query parameters, and the existingNullParamValueoverride to"NULL"is correctly preserved.balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (6)
19-37: LGTM! Clear documentation of the dual parameter model.The class signature refactor to separate
orderedParamsandnamedParamsis well-documented. The scaladoc clearly explains the distinction between positioned and named parameters.
39-47: LGTM! SQL generation correctly handles both parameter types.The SQL generation properly:
- Renders positioned parameters using their
sqlEntryvalues- Formats named parameters as
name := valueper PostgreSQL syntax- Maintains proper ordering (positioned parameters precede named parameters)
94-97: LGTM! Parameter aggregation maintains proper ordering.The
executemethod correctly concatenatesorderedParams.values ++ namedParams.values, ensuring positioned parameters are bound before named parameters.
107-130: LGTM! Parameter mutation methods maintain immutability.Both
setParamandclearcorrectly return new instances, preserving immutability. ThesetParammethod appropriately delegates tonamedParams.addand returnsDBFunctionWithNamedParamsToo.
146-152: LGTM! New factory methods improve API ergonomics.The new
applyoverloads provide convenient construction paths for functions with pre-built parameter collections, enhancing the API's usability.
196-199: Structure looks correct here.The
DBFunctionWithNamedParamsToodefinition appears syntactically correct. The pipeline error at line 200 is likely a cascading effect from the syntax error at line 162-163. Once that is fixed, this error should resolve.
| sealed case class DBFunctionWithPositionedParamsOnly private(functionName: String, | ||
| params: ParamsMap = ListMap.empty | ||
| ) extends DBFunction(functionName, params) { | ||
| orderedParams: OrderedParams = OrderedParams(), | ||
| ) extends DBFunction(functionName, orderedParams, namedParams = NamedParams()) { |
There was a problem hiding this comment.
Fix syntax error: Remove trailing comma.
The trailing comma on line 162 after OrderedParams() causes a compilation error because there are no additional parameters following it. This matches the pipeline failure: "identifier expected but ')' found."
🔎 Proposed fix
sealed case class DBFunctionWithPositionedParamsOnly private(functionName: String,
- orderedParams: OrderedParams = OrderedParams(),
+ orderedParams: OrderedParams = OrderedParams()
) extends DBFunction(functionName, orderedParams, namedParams = NamedParams()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sealed case class DBFunctionWithPositionedParamsOnly private(functionName: String, | |
| params: ParamsMap = ListMap.empty | |
| ) extends DBFunction(functionName, params) { | |
| orderedParams: OrderedParams = OrderedParams(), | |
| ) extends DBFunction(functionName, orderedParams, namedParams = NamedParams()) { | |
| sealed case class DBFunctionWithPositionedParamsOnly private(functionName: String, | |
| orderedParams: OrderedParams = OrderedParams() | |
| ) extends DBFunction(functionName, orderedParams, namedParams = NamedParams()) { |
🧰 Tools
🪛 GitHub Actions: Build
[error] 163-163: identifier expected but ')' found.
🤖 Prompt for AI Agents
In balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala around
lines 161 to 163, there's a trailing comma after OrderedParams() that causes a
syntax/compilation error; remove the trailing comma so the case class parameter
list becomes valid (i.e., delete the comma after OrderedParams() and ensure
spacing/indentation remains correct).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (1)
21-32: Consider marking ColumnName as final.The
ColumnNamecase class correctly overridesequalsandhashCodeto compare bysqlEntry, implementing SQL identifier equality semantics. However, consider marking itfinalto prevent unintended inheritance:-case class ColumnName (enteredName: String, +final case class ColumnName (enteredName: String,This signals that the class is not designed for extension and prevents potential equality contract violations in subclasses.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scalabalta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benedeki
Repo: AbsaOSS/balta PR: 61
File: balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala:153-155
Timestamp: 2025-12-31T11:12:57.166Z
Learning: In QueryResultRow, column names from metadata are stored as-is without normalization to avoid key collisions. Users must access columns using the same naming convention as in their queries: quoted identifiers (e.g., `"MyColumn"`) for mixed-case columns, unquoted for standard lowercase identifiers. This design preserves exact database column names and maintains query-to-result consistency.
📚 Learning: 2025-12-31T11:12:57.166Z
Learnt from: benedeki
Repo: AbsaOSS/balta PR: 61
File: balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala:153-155
Timestamp: 2025-12-31T11:12:57.166Z
Learning: In QueryResultRow, column names from metadata are stored as-is without normalization to avoid key collisions. Users must access columns using the same naming convention as in their queries: quoted identifiers (e.g., `"MyColumn"`) for mixed-case columns, unquoted for standard lowercase identifiers. This design preserves exact database column names and maintains query-to-result consistency.
Applied to files:
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala
🔇 Additional comments (4)
balta/src/main/scala/za/co/absa/db/mag/core/ColumnReference.scala (4)
19-19: LGTM: Clean trait definition.The
ColumnReferencetrait appropriately extendsSqlItemto provide thesqlEntrycontract for SQL identifier rendering.
34-45: LGTM: Regex patterns and helper methods are correct.The pattern definitions correctly distinguish between standard, mixed-case, quoted-regular, and quoted-arbitrary identifiers. The helper methods properly implement SQL quoting rules:
escapeQuotedoubles internal quotes per SQL standardquotewraps identifiers in double quoteshasUnescapedQuotescorrectly validates by removing escaped sequences firstAll previously reported regex bugs (missing
*quantifier) have been addressed.
47-66: Excellent implementation of SQL identifier semantics.The pattern matching logic correctly implements both PR objectives:
Case-insensitivity (Issue #1): Unquoted identifiers
abc,ABC, andAbCall producesqlEntry = "abc", making them equal per SQL standard.Quoting and escaping (Issue #2): Mixed-case or non-standard identifiers are properly quoted. Internal double quotes are correctly escaped using the SQL standard
""sequence.The distinction between unquoted and quoted identifiers is correct:
"abc"(unquoted) and"\"abc\""(quoted regular) → both resolve tosqlEntry = "abc"(equal, case-insensitive)"ABC"(unquoted) →sqlEntry = "abc"(lowercased)"\"ABC\""(quoted mixed-case) →sqlEntry = "\"ABC\""(preserved as case-sensitive)This aligns perfectly with SQL semantics where unquoted identifiers are case-insensitive but quoted identifiers preserve case.
68-79: LGTM: Clean positional parameter support and factory methods.
ColumnIndexcorrectly handles positional parameter references withsqlEntry = index.toString- The
privateconstructor onColumnIndexappropriately restricts instantiation to the companion objectColumnName.applyprovides a convenient delegation toColumnReference.apply
| override def values: Vector[QueryParamValue] = namedParams.toVector.map(_._2) | ||
| override def items: ParamsList = namedParams.toVector | ||
|
|
||
| def paramNames: Vector[ColumnName] = namedParams.toVector.map(_._1) |
There was a problem hiding this comment.
actually I'm just thinking why not to use word 'column' instead of 'param' here and for other places?
NamedParams -> NamedColumns or ColumnsNamed ?
OrderedParams -> OrderedColumns or ColumnsOrdered ?
| * then `None` is returned. | ||
| */ | ||
| def fieldValue[K: QueryParamType, T](keyName: String, keyValue: K, fieldName: String) | ||
| def fieldValue[K: QueryParamType, T](keyName: String, keyValue: K, columnName: String) |
There was a problem hiding this comment.
| def fieldValue[K: QueryParamType, T](keyName: String, keyValue: K, columnName: String) | |
| def columnValue[K: QueryParamType, T](keyName: String, keyValue: K, columnName: String) |
also the method description/doc can be changed slighlty
| } | ||
| object Params { | ||
|
|
||
| private type NamedParamsMap = ListMap[ColumnName, QueryParamValue] |
There was a problem hiding this comment.
ALso here etc, maybe we can name all this as 'Column' and not 'Params'
There was a problem hiding this comment.
(But I understand the Params logic as well, and it's not bad at all, Columns are just slightly more 'direct')
| DBFunctionWithPositionedParamsOnly(functionName) | ||
| } | ||
|
|
||
| def apply(functionName: String, params: NamedParams): DBFunctionWithNamedParamsToo = { |
There was a problem hiding this comment.
I like these two apply methods.
| */ | ||
| def clear(): DBFunctionWithPositionedParamsOnly = { | ||
| DBFunctionWithPositionedParamsOnly(functionName) | ||
| DBFunctionWithPositionedParamsOnly(functionName, OrderedParams()) |
There was a problem hiding this comment.
I find this method to be really confusing and cannot really see a nice clear usage. I'm actually not a big fan of some of this design at all, it seems rather complicated.
I like DBFunction - inheritance and API wise.
I think I like that it's private and you have 3 apply method in the companion object. But I don't like these two DBFunctionWithPositionedParamsOnly and DBFunctionWithNamedParamsToo at all, that's where the problems are arising.
I don't like name of DBFunctionWithNamedParamsToo - what is 'too'? (rhetorical question, ofc I know the answer) The class itself is not self-explanatory, some outside context is required, which builds mental dependencies. Unnecessary complexity.
Also, the 'direction' of code flow is just not simple. This is the core of the problem for me. You have DBFunction as a basis, then DBFunctionWithNamedParamsToo is its child, but the parent DBFunction is returning DBFunctionWithNamedParamsToo in one of the method. :(
There was a problem hiding this comment.
also the name clear is just too wide in my opinion
There was a problem hiding this comment.
What if DBFunction is just a trait? Containing very little method implementation, probably not even the sql method. Then the companion object would contain 1-3 apply methods (I prefer just 1 - why to make everything so rich and exhaustive?), something like:
def apply(functionName: String, orderedParams: OrderedParams = OrderedParams(), namedParams: NamedParams = NamedParams()): DBFunction =
DBFunctionImpl(functionName, orderedParams, namedParams)see the use of default parameters - you already have apply methods for Named and Ordered Params for this.
And then you would have something like this, the implementation class within the companion object itself
private case class DBFunctionImpl(functionName: String, orderedParams: OrderedParams, namedParams: NamedParams) extends DBFunction { ... <method SQL would be implemented here> ... }There was a problem hiding this comment.
Based on my prompt, the AI generated something like this, take it as an idea of some pseudo-code but interesting in design
/**
* A class that represents a database function call. It can be used to execute a function and verify the result.
* There are two implementations of this class:
* - withPositionedParams: parameters are defined by their position only
* - withNamedParams: parameters are defined by their name only
*
* @param functionName - the name of the function
* @param orderedParams - the list of parameters identified by their position
* @param namedParams - the list of parameters identified by their name
*/
sealed trait DBFunction extends DBQuerySupport {
def perform()(implicit connection: DBConnection): Unit = execute("")(_ => ())
def getResult(orderBy: String = "")(implicit connection: DBConnection): List[QueryResultRow] = execute(orderBy)(_.toList)
def execute[R](verify: QueryResult => R)(implicit connection: DBConnection): R = execute("")(verify)
def execute[R](orderBy: String)(verify: QueryResult => R)(implicit connection: DBConnection): R
def setOrderedParam[T: QueryParamType](value: T): DBFunction
def setNamedParam[T: QueryParamType](name: String, value: T): DBFunction
def clear(): DBFunction
}
object DBFunction {
def apply(functionName: String, orderedParams: OrderedParams = OrderedParams(), namedParams: NamedParams = NamedParams()): DBFunction =
DBFunctionImpl(functionName, orderedParams, namedParams)
private case class DBFunctionImpl(functionName: String, orderedParams: OrderedParams, namedParams: NamedParams) extends DBFunction {
private def sql(orderBy: String): String = {
val orderedEntries = orderedParams.values.map(_.sqlEntry)
val namedEntries = namedParams.items.map { case (columnName, queryParamValue) =>
columnName.sqlEntry + " := " + queryParamValue.sqlEntry
}
val paramsLine = (orderedEntries ++ namedEntries).mkString(",")
s"SELECT * FROM $functionName($paramsLine) $orderBy"
}
override def setOrderedParam[T: QueryParamType](value: T): DBFunction =
copy(orderedParams = orderedParams.add(value))
override def setNamedParam[T: QueryParamType](name: String, value: T): DBFunction =
copy(namedParams = namedParams.add(name, value))
override def execute[R](orderBy: String)(verify: QueryResult => R)(implicit connection: DBConnection): R = {
val orderByPart = if (orderBy.nonEmpty) s"ORDER BY $orderBy" else ""
runQuery(sql(orderByPart), orderedParams.values ++ namedParams.values)(verify)
}
override def clear(): DBFunction = copy(orderedParams = OrderedParams(), namedParams = NamedParams())
}
}There was a problem hiding this comment.
I will think about the rest, but the AI suggestion is fundamentally wrong.
It allows to add positioned (ordered) parameters after you have added a named one - that is incorrect. Once you add a named parameter only named ones can follow. That's why DBFunction is just a shell, and have two (hidden) children.
The other comments might have their point - but they are somewhat getting into the territory of really refactoring DBFunction - which is not the goal of this PR, and interacts with my plans for further improvements here (I know, I haven't voice them, and even less put them into a ticket). Which of course doesn't diminish their validity (definitely the naming could be better, I accept that)
There was a problem hiding this comment.
It allows to add positioned (ordered) parameters after you have added a named one - that is incorrect
Ok, then it's probably necessary to create some sub-entities of DBFunction and treat it separately. Maybe. I will think about it
The other comments might have their point - but they are somewhat getting into the territory of really refactoring DBFunction - which is not the goal of this PR, and interacts with my plans for further improvements here (I know, I haven't voice them, and even less put them into a ticket)
Hmm I partially understand/agree, but it wasn't possible to strongly voice this earlier - you introduced orderedParams and namedParams only now, instead of ParamsMap in the DBFunction. But ofc DBFunctionWithNamedParamsToo was present earlier and I haven't noticed / thought about it this way
ColumnReferenceandColumnNameto represent column and parameter names and handle them properly in SQL mannerParamsclass hierarchy to work with the new classes and for better structureSqlItemtrait shared between all classes providingSQLEntryDBTable,DBFunctionandDBQuerySupportnow utilize the streamlinedParamsstructureOverview
Release Notes
Related
Closes #1
Closes #2
Summary by CodeRabbit
New Features
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.