Skip to content

Conversation

@avelino
Copy link
Member

@avelino avelino commented Apr 5, 2025

This commit adds a welcome message that is sent to clients when they first connect to the ChronDB SQL server. The message is implemented using the PostgreSQL protocol's Notice Response message type.

Changes:

  • Added PG_NOTICE_RESPONSE constant for notice messages
  • Implemented send-notice-response function to send notice messages
  • Added welcome message sending in the client connection handler

The welcome message helps users identify they are connected to ChronDB and provides a better user experience by confirming the connection was successful.

example:

psql -h localhost -p 5432 -d chrondb -U chrondb -c "select sum(id) from user"
NOTICE:  Welcome to ChronDB - Git-backed Versioned Database System
 sum
-----
 0
(1 row)

Summary by CodeRabbit

  • New Features

    • Users now receive a welcome greeting ("Welcome to ChronDB - Git-backed Versioned Database System") immediately upon connecting.
    • The system now provides enhanced notice messages, improving overall client communications.
  • Tests

    • Comprehensive new test suites have been added to validate the functionality of aggregate operations, document handling, and SQL-like query processing, ensuring robust performance across various use cases.

This commit adds a welcome message that is sent to clients when they first connect to the ChronDB SQL server. The message is implemented using the PostgreSQL protocol's Notice Response message type.

Changes:
- Added PG_NOTICE_RESPONSE constant for notice messages
- Implemented send-notice-response function to send notice messages
- Added welcome message sending in the client connection handler

The welcome message helps users identify they are connected to ChronDB and provides a better user experience by confirming the connection was successful.

example:
```bash
psql -h localhost -p 5432 -d chrondb -U chrondb -c "select sum(id) from user"
NOTICE:  Welcome to ChronDB - Git-backed Versioned Database System
 sum
-----
 0
(1 row)
```

Signed-off-by: Avelino <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 5, 2025

📝 Walkthrough

Walkthrough

This change introduces notice response handling within ChronDB’s PostgreSQL protocol. A new constant for notice responses is added, and a corresponding function to construct and send notice messages is implemented. The client handler now uses this function to deliver a welcome notice message during the connection sequence, preserving existing logic while extending protocol capabilities.

Changes

Files Change Summary
src/chrondb/.../constants.clj, src/chrondb/.../messages.clj Added a new constant PG_NOTICE_RESPONSE and implemented the send-notice-response function, which constructs a notice response message using a ByteBuffer and sends it via write-message.
src/chrondb/.../handlers.clj Updated the handle-client function to invoke send-notice-response with a welcome message before calling send-ready-for-query, integrating notice responses into the connection flow.
test/chrondb/.../functions_test.clj, test/chrondb/.../operators_test.clj, test/chrondb/.../query_test.clj Introduced new test files and defined multiple test cases to validate the functionality of aggregate functions, operators, and SQL-like query handling, ensuring expected behavior across various scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant HC as Handle-Client
    participant NR as send-notice-response
    participant WM as Write-Message

    C->>HC: Connect / Handshake
    HC->>NR: Call send-notice-response with welcome message
    NR->>WM: Build and dispatch notice message (using PG_NOTICE_RESPONSE)
    WM->>C: Deliver notice response
    HC->>C: Send Ready-for-Query
Loading

Poem

I'm a rabbit, leaping through the code,
Bringing notices on a bright new road.
A welcome message hops with glee,
Sprinkling cheer from A to Z.
In this digital burrow, joy's in the air –
Hop along, dear friend, with care!
🐰✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 286be2c and 5413570.

📒 Files selected for processing (2)
  • test/chrondb/api/sql/execution/functions_test.clj (1 hunks)
  • test/chrondb/api/sql/execution/query_test.clj (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/chrondb/api/sql/execution/query_test.clj
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test
  • GitHub Check: test
🔇 Additional comments (4)
test/chrondb/api/sql/execution/functions_test.clj (4)

5-28: Well-structured aggregate result processing tests with comprehensive coverage

The test cases for process-aggregate-result function are well-structured and cover both normal scenarios and edge cases where the result is nil. Good practice to verify that count, sum, and avg functions handle nil inputs gracefully by returning appropriate zero values.


30-34: Good implementation of floating-point comparison helper

The approximately= helper function is a good solution for comparing floating-point numbers. Using a small delta (0.0001) avoids precision issues that could cause flaky tests.


36-65: Well-designed tests with comprehensive coverage of aggregate functions

The test cases cover all expected aggregate operations (count, sum, avg, min, max) with both normal and edge cases. The test data includes a mix of data types and a nil value case, which helps ensure robust function behavior.

I notice you've effectively addressed the previous review comments about numeric type comparisons by using the approximately= helper function on lines 53, 56, 59, and 62. This approach correctly handles the comparison between different numeric representations (rationals, integers, and floating-point values).


61-62:

Details

✅ Verification successful

Verify expected behavior for id field extraction

This test appears to sum numeric values extracted from id fields of format "user:n". Confirm that the expected sum of 15 (1+2+3+4+5) is correct and aligns with the actual implementation in the execute-aggregate-function.


🏁 Script executed:

#!/bin/bash
# Find the implementation of execute-aggregate-function to verify id extraction logic
rg -U "defn execute-aggregate-function" --type clojure

Length of output: 133


Review Update: Numeric Extraction Confirmed

The implementation in src/chrondb/api/sql/execution/functions.clj has been verified. The logic correctly extracts the numeric portion from id fields formatted as "user:n" and sums them (i.e. 1+2+3+4+5 equals 15), which aligns with the expected behavior. No further changes are needed.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/chrondb/api/sql/protocol/messages.clj (1)

150-176: Well-implemented notice response function.

The implementation of send-notice-response follows the same pattern as the existing send-error-response function and correctly formats the message according to the PostgreSQL protocol. The function properly:

  1. Sets the severity field to "NOTICE"
  2. Includes the message content
  3. Adds required null terminators
  4. Uses the appropriate message type constant

Consider extracting the common structure between send_error_response and send_notice_response into a shared helper function, as they share very similar implementation patterns. This would reduce code duplication and make future maintenance easier.

+;; Helper function for creating error and notice messages
+(defn- create-message-with-severity-and-text
+  [severity message]
+  (let [buffer (ByteBuffer/allocate 1024)
+        ;; Add severity field
+        _ (.put buffer (byte (int \S)))
+        _ (.put buffer (.getBytes severity StandardCharsets/UTF_8))
+        _ (.put buffer (byte 0))  ;; Null terminator
+
+        ;; Message field
+        _ (.put buffer (byte (int \M)))
+        _ (.put buffer (.getBytes message StandardCharsets/UTF_8))
+        _ (.put buffer (byte 0))  ;; Null terminator
+
+        ;; Final null terminator for the entire message
+        _ (.put buffer (byte 0))
+
+        pos (.position buffer)
+        final-data (byte-array pos)]
+    (.flip buffer)
+    (.get buffer final-data)
+    final-data))
+
 (defn send-error-response
   [^OutputStream out message]
-  (let [buffer (ByteBuffer/allocate 1024)  ;; Aumentar para 1KB, suficiente para a maioria das mensagens de erro
-        ;; Add error fields
-        _ (.put buffer (byte (int \S)))  ;; Severity field
-        _ (.put buffer (.getBytes "ERROR" StandardCharsets/UTF_8))
-        _ (.put buffer (byte 0))  ;; Null terminator
-
-        ;; Message field
-        _ (.put buffer (byte (int \M)))  ;; Message field
-        _ (.put buffer (.getBytes message StandardCharsets/UTF_8))
-        _ (.put buffer (byte 0))  ;; Null terminator
-
-        ;; Final null terminator for the entire message
-        _ (.put buffer (byte 0))
-
-        pos (.position buffer)
-        final-data (byte-array pos)]
-    (.flip buffer)
-    (.get buffer final-data)
-    (write-message out constants/PG_ERROR_RESPONSE final-data)))
+  (let [final-data (create-message-with-severity-and-text "ERROR" message)]
+    (write-message out constants/PG_ERROR_RESPONSE final-data)))

 (defn send-notice-response
   [^OutputStream out message]
-  (let [buffer (ByteBuffer/allocate 1024)
-        ;; Add notice fields
-        _ (.put buffer (byte (int \S)))  ;; Severity field
-        _ (.put buffer (.getBytes "NOTICE" StandardCharsets/UTF_8))
-        _ (.put buffer (byte 0))  ;; Null terminator
-
-        ;; Message field
-        _ (.put buffer (byte (int \M)))  ;; Message field
-        _ (.put buffer (.getBytes message StandardCharsets/UTF_8))
-        _ (.put buffer (byte 0))  ;; Null terminator
-
-        ;; Final null terminator for the entire message
-        _ (.put buffer (byte 0))
-
-        pos (.position buffer)
-        final-data (byte-array pos)]
-    (.flip buffer)
-    (.get buffer final-data)
-    (write-message out constants/PG_NOTICE_RESPONSE final-data)))
+  (let [final-data (create-message-with-severity-and-text "NOTICE" message)]
+    (write-message out constants/PG_NOTICE_RESPONSE final-data)))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 973dfec and 6b7122e.

📒 Files selected for processing (3)
  • src/chrondb/api/sql/protocol/constants.clj (1 hunks)
  • src/chrondb/api/sql/protocol/handlers.clj (1 hunks)
  • src/chrondb/api/sql/protocol/messages.clj (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test
  • GitHub Check: test
🔇 Additional comments (2)
src/chrondb/api/sql/protocol/constants.clj (1)

9-9: Consistent and appropriate constant declaration.

The new PG_NOTICE_RESPONSE constant follows the existing pattern for message type constants and is correctly defined as byte value for character 'N', which aligns with the PostgreSQL protocol specification for notice messages.

src/chrondb/api/sql/protocol/handlers.clj (1)

115-117: Well-positioned welcome message implementation.

The welcome message is appropriately placed in the connection sequence - after authentication and parameter status messages but before indicating readiness for queries. This ensures clients receive the welcome notice as part of the initial connection handshake.

This commit adds comprehensive test coverage for the SQL execution layer,
including:

- Operators tests (operators_test.clj):
  - Condition evaluation (equals, not equals, greater/less than, LIKE)
  - WHERE clause filtering
  - GROUP BY functionality
  - ORDER BY sorting
  - LIMIT application

- Functions tests (functions_test.clj):
  - Aggregate function result processing (COUNT, SUM, AVG)
  - Handling of nil results
  - Proper field naming in results

- Query tests (query_test.clj):
  - Basic query execution
  - Query result formatting
  - Error handling

These tests ensure the SQL execution layer correctly processes and transforms
queries according to PostgreSQL wire protocol specifications.

Signed-off-by: Avelino <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (5)
test/chrondb/api/sql/execution/functions_test.clj (1)

1-59: Consider adding tests for the new notice response functionality.

This file contains good tests for SQL aggregate functions, but the PR description mentions adding a welcome message functionality using notice responses. Consider adding tests for the new send-notice-response function and welcome message functionality to ensure they work as expected.

Would you like me to suggest some test cases for the welcome message functionality?

🧰 Tools
🪛 GitHub Actions: Tests

[error] 47-47: FAIL in (test-execute-aggregate-function) avg function: expected: (= 29.5 (functions/execute-aggregate-function :avg test-docs "age")), actual: (not (= 29.5 59/2))


[error] 50-50: FAIL in (test-execute-aggregate-function) min function: expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")), actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) max function: expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")), actual: (not (= 35.0 35))


[error] 56-56: FAIL in (test-execute-aggregate-function) numeric extraction from id field: expected: (= 15 (functions/execute-aggregate-function :sum test-docs "id")), actual: (not (= 15 15.0))

🪛 GitHub Actions: Coverage

[error] 47-47: FAIL in (test-execute-aggregate-function) - avg function expected: (= 29.5 (functions/execute-aggregate-function :avg test-docs "age")) actual: (not (= 29.5 59/2))


[error] 50-50: FAIL in (test-execute-aggregate-function) - min function expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")) actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) - max function expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")) actual: (not (= 35.0 35))


[error] 56-56: FAIL in (test-execute-aggregate-function) - numeric extraction from id field expected: (= 15 (functions/execute-aggregate-function :sum test-docs "id")) actual: (not (= 15 15.0))

test/chrondb/api/sql/execution/operators_test.clj (3)

39-42: Consider adding case-insensitive LIKE tests

While the LIKE operator tests cover patterns with % at the beginning, middle, and end positions, it would be beneficial to add tests for case-insensitive matching behavior which is common in SQL implementations.

 (testing "LIKE operator"
   (is (operators/evaluate-condition doc {:field "name" :op "like" :value "Al%"}))
   (is (operators/evaluate-condition doc {:field "name" :op "like" :value "%ice"}))
   (is (operators/evaluate-condition doc {:field "name" :op "like" :value "%lic%"}))
+  (is (operators/evaluate-condition doc {:field "name" :op "like" :value "al%"}))
+  (is (operators/evaluate-condition doc {:field "name" :op "like" :value "%ICE"}))
   (is (not (operators/evaluate-condition doc {:field "name" :op "like" :value "Bob%"}))))

72-92: Enhance group-by tests with more specific assertions

While the current tests verify the count of groups and that documents within groups share the same value for the grouped field, it would be more robust to verify the exact content of each group.

 (testing "Group by single field"
   (let [groups (operators/group-docs-by docs [{:column "dept"}])
         expected-count 2]  ;; IT and HR groups
     (is (= expected-count (count groups)))
     ;; Each group should have docs with the same dept
     (doseq [group groups]
-      (is (apply = (map :dept group))))))
+      (is (apply = (map :dept group))))
+    ;; Verify specific content of each group
+    (let [it-group (first (filter #(= "IT" (:dept (first %))) groups))
+          hr-group (first (filter #(= "HR" (:dept (first %))) groups))]
+      (is (= #{"user:1" "user:3"} (set (map :id it-group))))
+      (is (= #{"user:2" "user:4"} (set (map :id hr-group)))))))

1-3: Consider adding test for null/missing field values

To increase test robustness, consider adding test cases for documents with missing fields or nil values to ensure the operators handle these edge cases correctly.

 (ns chrondb.api.sql.execution.operators-test
   (:require [clojure.test :refer [deftest is testing]]
             [chrondb.api.sql.execution.operators :as operators]))
+
+(deftest test-evaluate-condition-with-nil-values
+  (let [doc-with-nil {:id "user:5"
+                      :name nil
+                      :age 40}
+        doc-with-missing {:id "user:6"}]
+
+    (testing "Conditions on nil values"
+      (is (operators/evaluate-condition doc-with-nil {:field "name" :op "=" :value nil}))
+      (is (not (operators/evaluate-condition doc-with-nil {:field "name" :op "!=" :value nil}))))
+
+    (testing "Conditions on missing fields"
+      (is (not (operators/evaluate-condition doc-with-missing {:field "name" :op "=" :value "Alice"})))
+      (is (not (operators/evaluate-condition doc-with-missing {:field "age" :op ">" :value "30"}))))))
test/chrondb/api/sql/execution/query_test.clj (1)

7-13: Consider adding department field to all users in mock data

To avoid the GROUP BY test failure, you could also modify the test data to ensure all users have a department field:

(defn create-mock-storage []
  (let [documents (atom {"user:1" {:id "user:1" :name "Alice" :age 30 :active true}
                         "user:2" {:id "user:2" :name "Bob" :age 25 :active false}
                         "user:3" {:id "user:3" :name "Charlie" :age 35 :active true :dept "IT"}
                         "user:4" {:id "user:4" :name "Diana" :age 28 :active false :dept "HR"}
+                         "user:1" {:id "user:1" :name "Alice" :age 30 :active true :dept "Marketing"}
+                         "user:2" {:id "user:2" :name "Bob" :age 25 :active false :dept "Sales"}
                         "product:1" {:id "product:1" :name "Laptop" :price 1200 :stock 10}
                         "product:2" {:id "product:2" :name "Phone" :price 800 :stock 20}})]

This approach would maintain the test expectation of exactly 2 departments (since there would be no null departments).

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 6b7122e and bd194f4.

📒 Files selected for processing (3)
  • test/chrondb/api/sql/execution/functions_test.clj (1 hunks)
  • test/chrondb/api/sql/execution/operators_test.clj (1 hunks)
  • test/chrondb/api/sql/execution/query_test.clj (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
test/chrondb/api/sql/execution/functions_test.clj

[error] 47-47: FAIL in (test-execute-aggregate-function) avg function: expected: (= 29.5 (functions/execute-aggregate-function :avg test-docs "age")), actual: (not (= 29.5 59/2))


[error] 50-50: FAIL in (test-execute-aggregate-function) min function: expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")), actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) max function: expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")), actual: (not (= 35.0 35))


[error] 56-56: FAIL in (test-execute-aggregate-function) numeric extraction from id field: expected: (= 15 (functions/execute-aggregate-function :sum test-docs "id")), actual: (not (= 15 15.0))

test/chrondb/api/sql/execution/query_test.clj

[error] 127-127: FAIL in (test-handle-select-with-group-by) SELECT with GROUP BY: expected: (= 2 (count results)), actual: (not (= 2 3))


[error] 129-129: FAIL in (test-handle-select-with-group-by) SELECT with GROUP BY: expected: (every? (fn* [p1__2831#] (and (:dept p1__2831#) (contains? p1__2831# :count_))) results), actual: (not (every? #object[chrondb.api.sql.execution.query_test$fn__2832$fn__2837 0x38d525aa "chrondb.api.sql.execution.query_test$fn__2832$fn__2837@38d525aa"] [{:dept nil, :count_ 2} {:dept "IT", :count_* 1} {:dept "HR", :count_* 1}]))

🪛 GitHub Actions: Coverage
test/chrondb/api/sql/execution/functions_test.clj

[error] 47-47: FAIL in (test-execute-aggregate-function) - avg function expected: (= 29.5 (functions/execute-aggregate-function :avg test-docs "age")) actual: (not (= 29.5 59/2))


[error] 50-50: FAIL in (test-execute-aggregate-function) - min function expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")) actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) - max function expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")) actual: (not (= 35.0 35))


[error] 56-56: FAIL in (test-execute-aggregate-function) - numeric extraction from id field expected: (= 15 (functions/execute-aggregate-function :sum test-docs "id")) actual: (not (= 15 15.0))

test/chrondb/api/sql/execution/query_test.clj

[error] 127-127: FAIL in (test-handle-select-with-group-by) - expected: (= 2 (count results)) actual: (not (= 2 3))


[error] 129-129: FAIL in (test-handle-select-with-group-by) - expected: (every? (fn* [p1__28958#] (and (:dept p1__28958#) (contains? p1__28958# :count_))) results) actual: (not (every? #object[chrondb.api.sql.execution.query_test$fn__28959$fn__28964 0x3b89589a "chrondb.api.sql.execution.query_test$fn__28959$fn__28964@3b89589a"] [{:dept nil, :count_ 2} {:dept "IT", :count_* 1} {:dept "HR", :count_* 1}]))

🪛 GitHub Actions: Lint project with clj-kondo
test/chrondb/api/sql/execution/query_test.clj

[warning] 33-33: inline def

🔇 Additional comments (17)
test/chrondb/api/sql/execution/functions_test.clj (3)

1-28: LGTM! Process aggregate result tests are well-structured.

The tests for process-aggregate-result function are comprehensive, covering various aggregate operations (count, sum, avg) and properly testing nil value handling. The expected outputs and assertions are appropriate.


37-42: LGTM! Count function tests look good.

The tests for the count function correctly verify both counting all documents and counting non-null values for a specific field.


58-59: LGTM! Unsupported function test is correct.

The test correctly verifies that unsupported aggregate functions return nil.

test/chrondb/api/sql/execution/operators_test.clj (5)

1-45: Well-structured test suite for condition evaluation

The test-evaluate-condition suite comprehensively tests various SQL comparison operators including equality, inequality, numeric comparisons, and pattern matching with LIKE. The test cases are well-organized with clear contexts using the testing macro.


47-70: Good coverage for where condition application

The test suite for apply-where-conditions properly verifies empty conditions, single conditions, multiple conditions (AND logic), and LIKE filtering. The assertions check both the count and the specific document IDs that should be returned.


94-114: Comprehensive sorting test cases

The test-sort-docs-by suite validates empty orders, single field sorting in both ascending and descending order, and multi-field sorting. The assertions properly verify the expected ordering of documents by checking their IDs.


116-137: Complete limit operation test coverage

The test-apply-limit suite covers all important edge cases: nil limit, zero limit, limit less than total count, limit equal to total count, and limit greater than total count. The assertions appropriately verify both the count and specific document IDs.


1-137:

Details

❓ Verification inconclusive

Request clarification on relationship to PR objective

The PR summary indicates this is about adding a welcome message on client connection, but this file adds tests for SQL operators. Could you clarify how these tests relate to the welcome message functionality mentioned in the PR objectives?


🏁 Script executed:

#!/bin/bash
# Search for changes related to notice responses or welcome messages
echo "Searching for notice response or welcome message implementations..."
rg -l "welcome.*message|notice.*response" --type clojure

Length of output: 330


Clarification Needed on Test Relevance to PR Objective

The PR summary indicates an update for adding a welcome message on client connection, yet this test file focuses solely on SQL operator functionality. I noticed that implementations related to welcome messages appear to reside in files such as src/chrondb/api/sql/protocol/constants.clj, messages.clj, and handlers.clj. Could you please clarify how these SQL operator tests are connected to the welcome message improvements mentioned in the PR objectives?

test/chrondb/api/sql/execution/query_test.clj (9)

1-5: LGTM! Namespace declaration and requirements look good.

The namespace declaration correctly requires the necessary dependencies for testing SQL query execution functionality.


38-49: LGTM! Well-written select-all test.

This test correctly verifies that all users are returned when executing a select-all query. The assertions check both the count and the set of IDs to ensure correctness.


51-62: LGTM! Good test coverage for WHERE clause filtering.

This test properly validates that the WHERE clause filters users correctly, ensuring only those with age > 28 are returned.


64-75: LGTM! Thorough test for ORDER BY functionality.

This test verifies that users are correctly sorted by age in descending order, checking both the count and the exact order of results.


77-88: LGTM! Well-structured test for LIMIT clause.

This test effectively validates that the LIMIT clause restricts the number of results returned, while maintaining the correct sorting order.


90-101: LGTM! Good test for column selection.

This test properly checks that only the specified columns (name and age) are included in the results when specific columns are requested.


103-113: LGTM! Clear test for ID-based selection.

This test effectively verifies that selecting a record by ID works correctly, checking both the count and the ID of the returned record.


131-145: LGTM! Comprehensive test for insert operation.

This test properly validates that a document is inserted and can be retrieved afterward, ensuring the insert operation functions correctly.


147-166: LGTM! Good validation of update functionality.

This test correctly verifies that an update operation modifies the specified fields while preserving others, and confirms the changes persist in storage.

Comment on lines 49 to 53
(testing "min function"
(is (= 25.0 (functions/execute-aggregate-function :min test-docs "age"))))

(testing "max function"
(is (= 35.0 (functions/execute-aggregate-function :max test-docs "age"))))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix numeric type comparisons in min/max function tests.

The tests for min and max are failing due to comparing floating point numbers with integers. The function returns integers (25, 35) while the tests expect floating point (25.0, 35.0).

- (is (= 25.0 (functions/execute-aggregate-function :min test-docs "age"))))
+ (is (= 25 (functions/execute-aggregate-function :min test-docs "age"))))

- (is (= 35.0 (functions/execute-aggregate-function :max test-docs "age"))))
+ (is (= 35 (functions/execute-aggregate-function :max test-docs "age"))))
📝 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.

Suggested change
(testing "min function"
(is (= 25.0 (functions/execute-aggregate-function :min test-docs "age"))))
(testing "max function"
(is (= 35.0 (functions/execute-aggregate-function :max test-docs "age"))))
(testing "min function"
(is (= 25 (functions/execute-aggregate-function :min test-docs "age"))))
(testing "max function"
(is (= 35 (functions/execute-aggregate-function :max test-docs "age"))))
🧰 Tools
🪛 GitHub Actions: Tests

[error] 50-50: FAIL in (test-execute-aggregate-function) min function: expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")), actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) max function: expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")), actual: (not (= 35.0 35))

🪛 GitHub Actions: Coverage

[error] 50-50: FAIL in (test-execute-aggregate-function) - min function expected: (= 25.0 (functions/execute-aggregate-function :min test-docs "age")) actual: (not (= 25.0 25))


[error] 53-53: FAIL in (test-execute-aggregate-function) - max function expected: (= 35.0 (functions/execute-aggregate-function :max test-docs "age")) actual: (not (= 35.0 35))

Comment on lines 115 to 129
(deftest test-handle-select-with-group-by
(testing "SELECT with GROUP BY"
(let [query {:type :select
:table "user"
:columns [{:type :column :column "dept"}
{:type :aggregate-function :function :count :args ["*"]}]
:where nil
:group-by [{:column "dept"}]
:order-by nil
:limit nil}
results (query/handle-select test-storage query)]
;; Two departments: IT and HR
(is (= 2 (count results)))
;; Each result should have dept and count_* fields
(is (every? #(and (:dept %) (contains? % :count_*)) results)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix GROUP BY test to account for null departments

This test is failing because users 1 and 2 don't have a dept field in your test data, but the GROUP BY query includes them with a null department. The test expects only 2 results (IT and HR) but gets 3 (null, IT, and HR).

Either add departments to all users in the mock data or update the test to account for null departments:

(deftest test-handle-select-with-group-by
  (testing "SELECT with GROUP BY"
    (let [query {:type :select
                 :table "user"
                 :columns [{:type :column :column "dept"}
                           {:type :aggregate-function :function :count :args ["*"]}]
                 :where nil
                 :group-by [{:column "dept"}]
                 :order-by nil
                 :limit nil}
          results (query/handle-select test-storage query)]
-      ;; Two departments: IT and HR
-      (is (= 2 (count results)))
+      ;; Three results: null department, IT, and HR
+      (is (= 3 (count results)))
      ;; Each result should have dept and count_* fields
-      (is (every? #(and (:dept %) (contains? % :count_*)) results)))))
+      (is (every? #(contains? % :count_*) results)))))

The pipeline error shows exactly this issue with results being [{:dept nil, :count_* 2} {:dept "IT", :count_* 1} {:dept "HR", :count_* 1}].

📝 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.

Suggested change
(deftest test-handle-select-with-group-by
(testing "SELECT with GROUP BY"
(let [query {:type :select
:table "user"
:columns [{:type :column :column "dept"}
{:type :aggregate-function :function :count :args ["*"]}]
:where nil
:group-by [{:column "dept"}]
:order-by nil
:limit nil}
results (query/handle-select test-storage query)]
;; Two departments: IT and HR
(is (= 2 (count results)))
;; Each result should have dept and count_* fields
(is (every? #(and (:dept %) (contains? % :count_*)) results)))))
(deftest test-handle-select-with-group-by
(testing "SELECT with GROUP BY"
(let [query {:type :select
:table "user"
:columns [{:type :column :column "dept"}
{:type :aggregate-function :function :count :args ["*"]}]
:where nil
:group-by [{:column "dept"}]
:order-by nil
:limit nil}
results (query/handle-select test-storage query)]
;; Three results: null department, IT, and HR
(is (= 3 (count results)))
(is (every? #(contains? % :count_*) results)))))
🧰 Tools
🪛 GitHub Actions: Tests

[error] 127-127: FAIL in (test-handle-select-with-group-by) SELECT with GROUP BY: expected: (= 2 (count results)), actual: (not (= 2 3))


[error] 129-129: FAIL in (test-handle-select-with-group-by) SELECT with GROUP BY: expected: (every? (fn* [p1__2831#] (and (:dept p1__2831#) (contains? p1__2831# :count_))) results), actual: (not (every? #object[chrondb.api.sql.execution.query_test$fn__2832$fn__2837 0x38d525aa "chrondb.api.sql.execution.query_test$fn__2832$fn__2837@38d525aa"] [{:dept nil, :count_ 2} {:dept "IT", :count_* 1} {:dept "HR", :count_* 1}]))

🪛 GitHub Actions: Coverage

[error] 127-127: FAIL in (test-handle-select-with-group-by) - expected: (= 2 (count results)) actual: (not (= 2 3))


[error] 129-129: FAIL in (test-handle-select-with-group-by) - expected: (every? (fn* [p1__28958#] (and (:dept p1__28958#) (contains? p1__28958# :count_))) results) actual: (not (every? #object[chrondb.api.sql.execution.query_test$fn__28959$fn__28964 0x3b89589a "chrondb.api.sql.execution.query_test$fn__28959$fn__28964@3b89589a"] [{:dept nil, :count_ 2} {:dept "IT", :count_* 1} {:dept "HR", :count_* 1}]))

avelino added 2 commits April 6, 2025 11:29
This change replaces the global test-storage var with a dynamic var *test-storage*
to better handle test isolation and prevent potential test interference. The
binding is now properly scoped within the test fixture.

Signed-off-by: Avelino <[email protected]>
This commit adds a helper function `approximately=` to handle floating-point
comparisons in aggregate function tests. This addresses potential issues with
floating-point precision when testing numeric results from functions like
avg, min, and max.

The change ensures more reliable test results by using a small delta value
(0.0001) to compare floating-point numbers, rather than exact equality which
can be problematic due to floating-point representation.

This improvement makes the tests more robust while maintaining their
intended behavior of verifying the correctness of aggregate function
calculations.

Signed-off-by: Avelino <[email protected]>
@avelino avelino merged commit 3d37534 into main Apr 6, 2025
9 checks passed
@avelino avelino deleted the avelino/sql-connect-send-notice-welcome branch April 6, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants