From 6c3aa580b5c919cb26f4aeb38903ecbf43ed0a8a Mon Sep 17 00:00:00 2001
From: Dustin Ray <40841027+drcapybara@users.noreply.github.com>
Date: Wed, 19 Jun 2024 13:05:10 -0700
Subject: [PATCH] fix: incorporate leftover feedback from previous PR (#17)

# Rationale for this change

Incorporates a small amount of feedback from #12

# What changes are included in this PR?

Corrects naming in tests and removes unsupported feature in
group_by_util

# Are these changes tested?

yes
---
 .../src/base/commitment/committable_column.rs    | 16 ++++++++--------
 crates/proof-of-sql/src/sql/ast/group_by_util.rs |  6 +++---
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/crates/proof-of-sql/src/base/commitment/committable_column.rs b/crates/proof-of-sql/src/base/commitment/committable_column.rs
index 546f303f5..a6fcb89dd 100644
--- a/crates/proof-of-sql/src/base/commitment/committable_column.rs
+++ b/crates/proof-of-sql/src/base/commitment/committable_column.rs
@@ -223,24 +223,24 @@ mod tests {
     #[test]
     fn we_can_get_type_and_length_of_timestamp_column() {
         // empty case
-        let smallint_committable_column =
+        let committable_column =
             CommittableColumn::TimestampTZ(PoSQLTimeUnit::Second, PoSQLTimeZone::UTC, &[]);
-        assert_eq!(smallint_committable_column.len(), 0);
-        assert!(smallint_committable_column.is_empty());
+        assert_eq!(committable_column.len(), 0);
+        assert!(committable_column.is_empty());
         assert_eq!(
-            smallint_committable_column.column_type(),
+            committable_column.column_type(),
             ColumnType::TimestampTZ(PoSQLTimeUnit::Second, PoSQLTimeZone::UTC)
         );
 
-        let smallint_committable_column = CommittableColumn::TimestampTZ(
+        let committable_column = CommittableColumn::TimestampTZ(
             PoSQLTimeUnit::Second,
             PoSQLTimeZone::UTC,
             &[12, 34, 56],
         );
-        assert_eq!(smallint_committable_column.len(), 3);
-        assert!(!smallint_committable_column.is_empty());
+        assert_eq!(committable_column.len(), 3);
+        assert!(!committable_column.is_empty());
         assert_eq!(
-            smallint_committable_column.column_type(),
+            committable_column.column_type(),
             ColumnType::TimestampTZ(PoSQLTimeUnit::Second, PoSQLTimeZone::UTC)
         );
     }
diff --git a/crates/proof-of-sql/src/sql/ast/group_by_util.rs b/crates/proof-of-sql/src/sql/ast/group_by_util.rs
index b874f128e..7477e98de 100644
--- a/crates/proof-of-sql/src/sql/ast/group_by_util.rs
+++ b/crates/proof-of-sql/src/sql/ast/group_by_util.rs
@@ -113,9 +113,9 @@ pub(super) fn sum_aggregate_column_by_index_counts<'a, S: Scalar>(
             todo!("aggregation over decimals not yet supported")
         }
         Column::Scalar(col) => sum_aggregate_slice_by_index_counts(alloc, col, counts, indexes),
-        Column::VarChar(_) => unimplemented!("Cannot sum varchar columns"),
-        Column::TimestampTZ(_, _, col) => {
-            sum_aggregate_slice_by_index_counts(alloc, col, counts, indexes)
+        Column::VarChar(_) => panic!("Cannot sum varchar columns"),
+        Column::TimestampTZ(_, _, _) => {
+            panic!("Cannot sum varchar columns")
         }
     }
 }