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 params into ordered and named containers, adds ColumnReference/SqlItem for normalized/quoted column identifiers, migrates internal param collections from List to Vector, renames field/metadata APIs to use "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 and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
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 | |
| } |
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.