Merge pull request #454 from metabase/add_implicit_fields_clause

Add implicit fields clause to "rows" aggregation queries

src/metabase/driver.clj

@@ -128,7 +128,8 @@
   [query]
   {:pre [(map? query)]}
   (try
-    (binding [qp/*query* query]
+    (binding [qp/*query* query
+              qp/*internal-context* (atom {})]
       (let [driver  (database-id->driver (:database query))
             query   (qp/preprocess query)
             results (binding [qp/*query* query]

src/metabase/driver/query_processor.clj

@@ -2,12 +2,14 @@
   "Preprocessor that does simple transformations to all incoming queries, simplifing the driver-specific implementations."
   (:require [clojure.core.match :refer [match]]
             [clojure.tools.logging :as log]
+            [korma.core :refer :all]
             [metabase.db :refer :all]
             [metabase.driver.interface :as i]
             [metabase.models.field :refer [Field field->fk-table]]))
 
 (declare add-implicit-breakout-order-by
          add-implicit-limit
+         add-implicit-fields
          get-special-column-info
          preprocess-cumulative-sum
          preprocess-structured
@@ -34,6 +36,10 @@
   "Should we disable logging for the QP? (e.g., during sync we probably want to turn it off to keep logs less cluttered)."
   false)
 
+(def ^:dynamic *internal-context*
+  "A neat place to store 'notes-to-self': things individual implementations don't need to know about, like if the `fields` clause was added implicitly."
+  (atom nil))
+
 
 ;; # PREPROCESSOR
 
@@ -51,6 +57,7 @@
                                                            remove-empty-clauses
                                                            add-implicit-breakout-order-by
                                                            add-implicit-limit
+                                                           add-implicit-fields
                                                            preprocess-cumulative-sum))]
     (when-not *disable-qp-logging*
       (log/debug (colorize.core/cyan "\n******************** PREPROCESSED: ********************\n"
@@ -102,7 +109,7 @@
 ;;; ### ADD-IMPLICIT-LIMIT
 
 (defn add-implicit-limit
-  "Add an implicit limit clause to queries with `rows` aggregations."
+  "Add an implicit `limit` clause to queries with `rows` aggregations."
   [{:keys [limit aggregation] :as query}]
   (if (and (= aggregation ["rows"])
            (not limit))
@@ -110,6 +117,21 @@
     query))
 
 
+;;; ### ADD-IMPLICIT-FIELDS
+
+(defn add-implicit-fields
+  "Add an implicit `fields` clause to queries with `rows` aggregations."
+  [{:keys [fields aggregation breakout source_table] :as query}]
+  (if-not (and (= aggregation ["rows"])
+               (not breakout)
+               (not fields))
+    query
+    ;; If we're doing a "rows" aggregation with no breakout or fields clauses add one that will exclude Fields that are supposed to be hidden
+    (do (swap! *internal-context* assoc :fields-is-implicit true)
+        (assoc query :fields (sel :many :id Field :table_id source_table, :active true, :preview_display true,
+                                  :field_type [not= "sensitive"], (order :position :asc), (order :id :desc))))))
+
+
 ;; ### PREPROCESS-CUMULATIVE-SUM
 
 (defn preprocess-cumulative-sum
@@ -131,7 +153,7 @@
       ;; to just fetch all the values of the field in question.
       cum-sum-with-same-breakout? (-> query
                                       (dissoc :breakout)
-                                      (assoc :cum_sum     ag-field
+                                      (assoc :cum_sum     ag-field     ; TODO - move this to *internal-context* instead?
                                              :aggregation ["rows"]
                                              :fields      [ag-field]))
 
@@ -320,7 +342,7 @@
   (try
     (-order-columns (sel :many :fields [Field :id :name :position] :table_id source-table)
                     breakout-field-ids
-                    field-field-ids
+                    (when-not (:fields-is-implicit @*internal-context*) field-field-ids)
                     castified-field-names)
     (catch Exception e
       (.printStackTrace e)

test/metabase/driver/generic_sql/query_processor_test.clj

@@ -12,7 +12,9 @@
 ;; Check that we get an error response formatted the way we'd expect
 (expect
     {:status :failed
-     :error (format "Column \"CHECKINS.NAME\" not found; SQL statement:\nSELECT \"CHECKINS\".* FROM \"CHECKINS\" WHERE (\"CHECKINS\".\"NAME\" = ?) LIMIT %d" max-result-rows)}
+     :error (str "Column \"CHECKINS.NAME\" not found; SQL statement:\nSELECT \"CHECKINS\".\"ID\", CAST(\"DATE\" AS DATE), "
+                 "\"CHECKINS\".\"VENUE_ID\", \"CHECKINS\".\"USER_ID\" FROM \"CHECKINS\" WHERE (\"CHECKINS\".\"NAME\" = ?) LIMIT "
+                 max-result-rows)}
   ;; This will print a stacktrace. Better to reassure people that that's on purpose than to make people question whether the tests are working
   (do (log/info (color/green "NOTE: The following stacktrace is expected <3"))
       (driver/process-query {:database @db-id

test/metabase/driver/query_processor_test.clj

@@ -4,7 +4,8 @@
             [metabase.db :refer :all]
             [metabase.driver :as driver]
             [metabase.driver.query-processor :refer :all]
-            (metabase.models [table :refer [Table]])
+            (metabase.models [field :refer [Field]]
+                             [table :refer [Table]])
             [metabase.test.data.datasets :as datasets :refer [*dataset*]]))
 
 ;; ##  Dataset-Independent Data Fns
@@ -706,3 +707,24 @@
    :aggregation  ["stddev" (id :venues :category_id)]
    :breakout     [(id :venues :price)]
    :order_by     [[["aggregation" 0] "descending"]]})
+
+;;; ### make sure that rows where preview_display = false don't get displayed
+
+(datasets/expect-with-all-datasets
+ [(set (->columns "category_id" "name" "latitude" "id" "longitude" "price"))
+  (set (->columns "category_id" "name" "latitude" "id" "longitude"))
+  (set (->columns "category_id" "name" "latitude" "id" "longitude" "price"))]
+ (let [get-col-names (fn [] (-> (driver/process-query {:database (db-id)
+                                                       :type     "query"
+                                                       :query    {:aggregation  ["rows"]
+                                                                  :source_table (id :venues)
+                                                                  :order_by     [[(id :venues :id) "ascending"]]
+                                                                  :limit        1}})
+                                :data
+                                :columns
+                                set))]
+   [(get-col-names)
+    (do (upd Field (id :venues :price) :preview_display false)
+        (get-col-names))
+    (do (upd Field (id :venues :price) :preview_display true)
+        (get-col-names))]))