[GitHub] [superset] arita37 commented on pull request #14449: feat: Add parquet upload

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

arita37 commented on pull request #14449:
URL: https://github.com/apache/superset/pull/14449#issuecomment-892261069


   It will be a very very useful features
   for many people.
   
   
   
   > On Aug 3, 2021, at 23:08, Ville Brofeldt ***@***.***> wrote:
   > 
   > 
   > @villebro requested changes on this pull request.
   > 
   > Great work! Just a few (hopefully) last comments - I'll prioritize seeing this through, so as soon as you can make the necessary updates let me know!
   > 
   > In superset/views/database/forms.py:
   > 
   > > +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
   > +            )
   > +        ]
   > 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.
   > 
   > In superset/templates/superset/form_view/columnar_to_database_view/edit.html:
   > 
   > > +    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",
   > 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.
   > 
   > In superset/views/database/forms.py:
   > 
   > > +    # pylint: disable=E0211
   > +    def columnar_allowed_dbs() -> List[Database]:  # type: ignore
   > I wonder why this wasn't @staticmethod?? Can we update that here and in the other classes
   > 
   > In superset/views/database/forms.py:
   > 
   > > +    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()],
   > +    )
   > Yes, let's keep this, I think it's a great addition 👍
   > 
   > —
   > You are receiving this because you commented.
   > Reply to this email directly, view it on GitHub, or unsubscribe.
   > Triage notifications on the go with GitHub Mobile for iOS or Android.
   


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