opus and codex review suggestions

ankitml · ankitml · commit d98dd844f2a5 · 2026-02-05T15:00:16.000-05:00
diff --git a/lib/parade_db.rb b/lib/parade_db.rb
@@ -5,4 +5,5 @@
 require_relative "parade_db/search_methods"
 
 module ParadeDB
+  class FacetQueryError < ArgumentError; end
 end
diff --git a/lib/parade_db/search_methods.rb b/lib/parade_db/search_methods.rb
@@ -103,7 +103,7 @@ def more_like_this(key, fields: nil)
 
     def with_score
       score_sql = %(pdb.score("#{table_name}"."#{primary_key}") AS search_score)
-      except(:select).select(::Arel.sql("#{table_name}.*"), ::Arel.sql(score_sql))
+      with_projection(score_sql)
     end
 
     def with_snippet(column, start_tag: nil, end_tag: nil, max_chars: nil)
@@ -118,7 +118,7 @@ def with_snippet(column, start_tag: nil, end_tag: nil, max_chars: nil)
                %(pdb.snippet("#{table_name}"."#{column}", #{formatted_args.join(', ')}))
              end
       
-      except(:select).select(::Arel.sql("#{table_name}.*"), ::Arel.sql("#{call} AS #{column}_snippet"))
+      with_projection("#{call} AS #{column}_snippet")
     end
 
     # ---- Facets ----
@@ -184,6 +184,12 @@ def ensure_paradedb_predicate
 
     private
 
+    def with_projection(sql_fragment)
+      rel = self
+      rel = rel.select(::Arel.sql("#{table_name}.*")) if rel.select_values.empty?
+      rel.select(::Arel.sql(sql_fragment))
+    end
+
     def where_values_as_sql
       # Extract WHERE clause SQL from the relation
       # For facet queries that need the predicate SQL
@@ -282,23 +288,52 @@ def parse_facets(row)
         row.each do |key, value|
           if key.end_with?("_facet")
             field_name = key.delete_suffix("_facet")
-            facets[field_name] = JSON.parse(value) if value
+            parsed = parse_facet_value(value)
+            facets[field_name] = parsed unless parsed.nil?
           end
         end
         facets
       end
+
+      def parse_facet_value(value)
+        case value
+        when nil
+          nil
+        when String
+          JSON.parse(value)
+        else
+          value
+        end
+      end
     end
 
     # Module to add .facets accessor to relations
     module FacetRelation
       attr_accessor :_paradedb_facet_fields, :_paradedb_facet_opts
 
+      def load(*)
+        validate_facet_query_shape!
+        super
+      end
+
       def facets
+        validate_facet_query_shape!
         @_facets_cache ||= extract_facets_from_results
       end
 
       private
 
+      def validate_facet_query_shape!
+        missing = []
+        missing << "ORDER BY" if order_values.empty?
+        missing << "LIMIT" if limit_value.nil?
+        return if missing.empty?
+
+        raise ParadeDB::FacetQueryError,
+              "with_facets requires #{missing.join(' and ')} for ParadeDB TopN pushdown. " \
+              "Use .order(...).limit(...)."
+      end
+
       def extract_facets_from_results
         # Execute query and extract facet columns
         first_row = limit(1).first
@@ -309,11 +344,23 @@ def extract_facets_from_results
           facet_col = "_#{field}_facet"
           if first_row.respond_to?(facet_col)
             value = first_row.public_send(facet_col)
-            facets[field.to_s] = JSON.parse(value) if value
+            parsed = parse_facet_value(value)
+            facets[field.to_s] = parsed unless parsed.nil?
           end
         end
         facets
       end
+
+      def parse_facet_value(value)
+        case value
+        when nil
+          nil
+        when String
+          JSON.parse(value)
+        else
+          value
+        end
+      end
     end
   end
 end
diff --git a/spec/user_api_behavior_integration_spec.rb b/spec/user_api_behavior_integration_spec.rb
@@ -0,0 +1,136 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+class BehaviorProduct < ActiveRecord::Base
+  include ParadeDB::Model
+  self.table_name = :products
+  self.has_paradedb_index = true
+end
+
+class UserApiBehaviorIntegrationTest < Minitest::Test
+  def setup
+    skip "Behavior integration tests require PostgreSQL" unless postgresql?
+
+    ensure_paradedb_setup!
+    seed_products!
+  end
+
+  def test_matching_all_executes_and_returns_rows
+    ids = BehaviorProduct.search(:description)
+                         .matching_all("running", "shoes")
+                         .order(:id)
+                         .pluck(:id)
+
+    assert_equal [1, 2], ids
+  end
+
+  def test_matching_any_executes_and_returns_rows
+    ids = BehaviorProduct.search(:description)
+                         .matching_any("wireless", "hiking")
+                         .order(:id)
+                         .pluck(:id)
+
+    assert_equal [3, 5], ids
+  end
+
+  def test_phrase_near_and_phrase_prefix_execute
+    phrase_ids = BehaviorProduct.search(:description).phrase("running shoes").order(:id).pluck(:id)
+    near_ids = BehaviorProduct.search(:description).near("running", "shoes", distance: 1).order(:id).pluck(:id)
+    prefix_ids = BehaviorProduct.search(:category).phrase_prefix("foot").order(:id).pluck(:id)
+
+    assert_equal [1, 2], phrase_ids
+    assert_equal [1, 2], near_ids
+    assert_equal [1, 2, 5], prefix_ids
+  end
+
+  def test_term_regex_and_fuzzy_execute
+    term_ids = BehaviorProduct.search(:category).term("audio").order(:id).pluck(:id)
+    regex_ids = BehaviorProduct.search(:description).regex("run.*").order(:id).pluck(:id)
+    fuzzy_ids = BehaviorProduct.search(:description).fuzzy("shose", distance: 2).order(:id).pluck(:id)
+
+    assert_equal [3, 4], term_ids
+    assert_equal [1, 2, 6], regex_ids
+    assert_equal [1, 2], fuzzy_ids
+  end
+
+  def test_more_like_this_executes_and_returns_similar_rows
+    ids = BehaviorProduct.more_like_this(3, fields: [:description]).limit(5).pluck(:id)
+
+    assert_includes ids, 4
+  end
+
+  def test_with_score_and_with_snippet_materialize_columns
+    rows = BehaviorProduct.search(:description)
+                          .matching_all("running shoes")
+                          .with_score
+                          .with_snippet(:description, start_tag: "<b>", end_tag: "</b>", max_chars: 60)
+                          .order(:id)
+                          .to_a
+
+    refute_empty rows
+    rows.each do |row|
+      refute_nil row.search_score
+      refute_nil row.description_snippet
+    end
+  end
+
+  def test_facets_and_with_facets_execute_and_parse_results
+    facet_hash = BehaviorProduct.search(:description).matching_all("earbuds").facets(:rating)
+    assert_kind_of Hash, facet_hash
+    assert_includes facet_hash, "rating"
+    assert_match(/[35]/, facet_hash["rating"].to_json)
+
+    rel = BehaviorProduct.search(:description)
+                         .matching_all("running shoes")
+                         .with_facets(:rating, size: 10)
+                         .order(:id)
+                         .limit(10)
+    rows = rel.to_a
+    assert_equal [1, 2], rows.map(&:id)
+
+    rel_facets = rel.facets
+    assert_kind_of Hash, rel_facets
+    assert_includes rel_facets, "rating"
+    assert_match(/[45]/, rel_facets["rating"].to_json)
+  end
+
+  def test_with_facets_without_topn_shape_raises_friendly_error
+    rel = BehaviorProduct.search(:description).matching_all("running shoes").with_facets(:rating, size: 10)
+    error = assert_raises(ParadeDB::FacetQueryError) { rel.to_a }
+    assert_includes error.message, "ORDER BY and LIMIT"
+  end
+
+  private
+
+  def postgresql?
+    ActiveRecord::Base.connection.adapter_name.downcase.include?("postgres")
+  end
+
+  def ensure_paradedb_setup!
+    return if self.class.instance_variable_get(:@paradedb_setup_done)
+
+    conn = ActiveRecord::Base.connection
+    conn.execute("CREATE EXTENSION IF NOT EXISTS pg_search;")
+    conn.execute("DROP INDEX IF EXISTS products_bm25_idx;")
+    conn.execute(<<~SQL)
+      CREATE INDEX products_bm25_idx ON products
+      USING bm25 (id, description, category, rating, in_stock, price)
+      WITH (key_field='id');
+    SQL
+
+    self.class.instance_variable_set(:@paradedb_setup_done, true)
+  end
+
+  def seed_products!
+    conn = ActiveRecord::Base.connection
+    conn.execute("TRUNCATE TABLE products RESTART IDENTITY;")
+
+    BehaviorProduct.create!(description: "running shoes lightweight", category: "footwear", rating: 5, in_stock: true, price: 120)
+    BehaviorProduct.create!(description: "trail running shoes grip", category: "footwear", rating: 4, in_stock: true, price: 90)
+    BehaviorProduct.create!(description: "wireless bluetooth earbuds", category: "audio", rating: 5, in_stock: true, price: 80)
+    BehaviorProduct.create!(description: "budget wired earbuds", category: "audio", rating: 3, in_stock: false, price: 20)
+    BehaviorProduct.create!(description: "hiking boots waterproof", category: "footwear", rating: 4, in_stock: true, price: 110)
+    BehaviorProduct.create!(description: "running socks breathable", category: "apparel", rating: 2, in_stock: true, price: 15)
+  end
+end
diff --git a/spec/user_api_integration_spec.rb b/spec/user_api_integration_spec.rb
@@ -175,6 +175,36 @@ def test_with_snippet_custom
     assert_sql_equal expected, sql
   end
 
+  def test_with_score_then_with_snippet_keeps_both_projections
+    sql = Product.search(:description)
+                 .matching_all("shoes")
+                 .with_score
+                 .with_snippet(:description)
+                 .to_sql
+
+    expected = <<~SQL.strip
+      SELECT products.*, pdb.score("products"."id") AS search_score, pdb.snippet("products"."description") AS description_snippet FROM products
+      WHERE ("products"."description" &&& 'shoes')
+    SQL
+
+    assert_sql_equal expected, sql
+  end
+
+  def test_with_snippet_then_with_score_keeps_both_projections
+    sql = Product.search(:description)
+                 .matching_all("shoes")
+                 .with_snippet(:description)
+                 .with_score
+                 .to_sql
+
+    expected = <<~SQL.strip
+      SELECT products.*, pdb.snippet("products"."description") AS description_snippet, pdb.score("products"."id") AS search_score FROM products
+      WHERE ("products"."description" &&& 'shoes')
+    SQL
+
+    assert_sql_equal expected, sql
+  end
+
   def test_or_across_fields
     base = Product.where(in_stock: true).order(id: :desc).limit(10)
     left = base.search(:description).matching_all("shoes")
diff --git a/spec/user_api_unit_spec.rb b/spec/user_api_unit_spec.rb
@@ -96,6 +96,28 @@ def test_with_snippet_custom
       WHERE ("products"."description" &&& 'shoes')), sql
   end
 
+  def test_with_score_then_with_snippet_keeps_both_projections
+    sql = UnitProduct.search(:description)
+                     .matching_all("shoes")
+                     .with_score
+                     .with_snippet(:description)
+                     .to_sql
+
+    assert_sql_equal %(SELECT products.*, pdb.score("products"."id") AS search_score, pdb.snippet("products"."description") AS description_snippet FROM products
+      WHERE ("products"."description" &&& 'shoes')), sql
+  end
+
+  def test_with_snippet_then_with_score_keeps_both_projections
+    sql = UnitProduct.search(:description)
+                     .matching_all("shoes")
+                     .with_snippet(:description)
+                     .with_score
+                     .to_sql
+
+    assert_sql_equal %(SELECT products.*, pdb.snippet("products"."description") AS description_snippet, pdb.score("products"."id") AS search_score FROM products
+      WHERE ("products"."description" &&& 'shoes')), sql
+  end
+
   def test_facets_only
     facet_sql = UnitProduct.search(:description).matching_all("shoes")
                            .build_facet_query(fields: [:category, :brand], size: 10, order: "-count")
@@ -158,6 +180,35 @@ def test_with_facets_without_paradedb_predicates
     assert_sql_equal expected, sql
   end
 
+  def test_with_facets_load_requires_order_and_limit
+    rel = UnitProduct.search(:description)
+                     .matching_all("shoes")
+                     .with_facets(:category, size: 10)
+
+    error = assert_raises(ParadeDB::FacetQueryError) { rel.load }
+    assert_includes error.message, "ORDER BY and LIMIT"
+  end
+
+  def test_with_facets_load_requires_limit
+    rel = UnitProduct.search(:description)
+                     .matching_all("shoes")
+                     .with_facets(:category, size: 10)
+                     .order(:id)
+
+    error = assert_raises(ParadeDB::FacetQueryError) { rel.load }
+    assert_includes error.message, "LIMIT"
+  end
+
+  def test_with_facets_load_requires_order
+    rel = UnitProduct.search(:description)
+                     .matching_all("shoes")
+                     .with_facets(:category, size: 10)
+                     .limit(10)
+
+    error = assert_raises(ParadeDB::FacetQueryError) { rel.load }
+    assert_includes error.message, "ORDER BY"
+  end
+
   def test_search_on_relation_preserves_where
     # Verify .search() works on relations and preserves WHERE clauses
     sql = UnitProduct.where(in_stock: true)
