[GitHub] [superset] villebro commented on a change in pull request #14449: feat: Add parquet upload

[GitBox <gi...@apache.org>]

villebro commented on a change in pull request #14449:
URL: https://github.com/apache/superset/pull/14449#discussion_r681785283



##########
File path: superset/views/database/forms.py
##########
@@ -402,3 +417,130 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
             'Use [""] for empty string.'
         ),
     )
+
+
+class ColumnarToDatabaseForm(DynamicForm):
+    # pylint: disable=E0211
+    def columnar_allowed_dbs() -> List[Database]:  # type: ignore
+        # TODO: change allow_csv_upload to allow_file_upload
+        columnar_enabled_dbs = (
+            db.session.query(Database).filter_by(allow_csv_upload=True).all()
+        )
+        return [
+            columnar_enabled_db
+            for columnar_enabled_db in columnar_enabled_dbs
+            if ColumnarToDatabaseForm.at_least_one_schema_is_allowed(
+                columnar_enabled_db
+            )
+        ]

Review comment:
       There's unnecessary duplication here: we could abstract this into a `class UploadToDatabaseForm(DynamicForm)` with all the repeated boilerplate and then extend those into `class CsvToDatabaseForm(UploadToDatabaseForm)`, `class ColumnarToDatabaseForm(UploadToDatabaseForm)` etc. But this was here before you already, so not your fault! But if you feel like doing some valuable cleanup work, it would be great to do some of this cleanup in a follow-up PR.

##########
File path: superset/templates/superset/form_view/columnar_to_database_view/edit.html
##########
@@ -0,0 +1,64 @@
+{#
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing,
+  software distributed under the License is distributed on an
+  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+  KIND, either express or implied.  See the License for the
+  specific language governing permissions and limitations
+  under the License.
+#}
+{% extends 'appbuilder/general/model/edit.html' %}
+
+{% block tail_js %}
+  {{ super() }}
+  <script>
+    var db = $("#con");
+    var schema = $("#schema");
+
+    // this element is a text input
+    // copy it here so it can be reused later
+    var any_schema_is_allowed = schema.clone();
+
+    update_schemas_allowed_for_columnar_upload(db.val());
+    db.change(function(){
+        update_schemas_allowed_for_columnar_upload(db.val());
+    });
+
+    function update_schemas_allowed_for_columnar_upload(db_id) {
+        $.ajax({
+          method: "GET",
+          url: "/superset/schemas_access_for_columnar_upload",

Review comment:
       This endpoint is called `schemas_access_for_csv_upload` and will cause a 404 (this same error happens in the Excel upload form) - either use that or rename the original to `schemas_access_for_file_upload`.

##########
File path: superset/views/database/forms.py
##########
@@ -402,3 +417,130 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
             'Use [""] for empty string.'
         ),
     )
+
+
+class ColumnarToDatabaseForm(DynamicForm):
+    # pylint: disable=E0211
+    def columnar_allowed_dbs() -> List[Database]:  # type: ignore

Review comment:
       I wonder why this wasn't `@staticmethod`?? Can we update that here and in the other classes

##########
File path: superset/views/database/forms.py
##########
@@ -163,6 +169,15 @@ def at_least_one_schema_is_allowed(database: Database) -> bool:
         _("Mangle Duplicate Columns"),
         description=_('Specify duplicate columns as "X.0, X.1".'),
     )
+    usecols = JsonListField(
+        _("Use Columns"),
+        default=None,
+        description=_(
+            "Json list of the column names that should be read. "
+            "If not None, only these columns will be read from the file."
+        ),
+        validators=[Optional()],
+    )

Review comment:
       Yes, let's keep this, I think it's a great addition 👍 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org