Improving performance of CSV/Excel upload to Microsoft SQL Server

[sfirke]

Summary: uploading a spreadsheet to the data warehouse via Superset is unsatisfactory in two ways:

1. It takes a long time
2. It eventually succeeds, but the frontend times out and serves an error message to the user, causing them to think the operation failed.

This post is about (1) but either way, there should be a better user experience to address (2). Could it be handled by a worker instead of the main Superset application?

Specifics:
My users think that their spreadsheet uploads fail because they get a gateway error message after 30 or 60 seconds. I benchmarked an upload of an .xlsx file using Superset 4.1.0rc2. It is 182kb file size with just over 5k rows and 4 columns of data -- 1 date and 3 float columns. Not a huge file.

Benchmarking:

• Upload from Superset into my data warehouse (Microsoft SQL Server): 250 seconds
• Writing this same data from a Pandas dataframe to SQL Server with Pandas to_sql, method = 'multi': 16 seconds
• Writing with to_sql and method = None: 250 seconds

Possible fix:
Poking around in the code base, it looks like if I changed the MSSQL db engine spec, I could get Superset to write with method = 'multi', see here:

superset/superset/db_engine_specs/base.py

Line 1286 in 5e42d7a

if engine.dialect.supports_multivalues_insert:

I guess I could just add supports_multivalues_insert = True to the SQL Server spec?

But I'm confused, why then does supports_multivalues_insert not appear in a single existing db engine spec as far as I see? The only idea I have is that all of the commonly-used DBs by developers override the df_to_sql function so don't care about this variable?

[nytai]

It's worth trying supports_multivalues_insert = True setting for mssql db engine spec. I suspect your reasoning as to why it's not enabled in more specs is correct, as the most of the commonly used ones are overriding that method. I believe csv uploads was a more common use case of superset in the past but more recently the advice has been to find other ways to get data into dbs since the csv upload process isn't very robust.

With the recent activity around async task frameworks it would be fairly simple to move csv upload to an async process and email the user once it's done.

[sfirke]

I added the last line here to my mssql.py:

class MssqlEngineSpec(BaseEngineSpec):
    engine = "mssql"
    engine_name = "Microsoft SQL Server"
    limit_method = LimitMethod.WRAP_SQL
    max_column_name_length = 128
    allows_cte_in_subquery = False
    allow_limit_clause = False
    supports_multivalues_insert = True

But it's not causing the method = "multi" to get sent to the to_sql call, I logged the kwargs of the to_sql call and it's absent:

superset_app          | 2024-09-05 19:35:22,822:INFO:root:to_sql_kwargs: {'chunksize': 1000, 'if_exists': 'replace', 'index': False, 'name': 'test_superset_fast_upload', 'schema': 'dbo'}

So either I added it in the wrong place for it to take effect, or the codebase is broken and it doesn't properly ingest and apply this setting...

[sfirke]

Maybe it was the latter?

This line was always returning False:
if engine.dialect.supports_multivalues_insert:

I changed it to:
if cls.supports_multivalues_insert:

And that value was True. I don't really know the difference in those two variables. It looks like SQLAlchemy sets supports_multivalues_insert to True if the MSSQL version is >2008 which mine is. So maybe engine.dialect is getting the general version for MSSQL (False) while cls. is getting the one for my particular instance? IDK.

Is this a bug in Superset that should be patched?

Edited to delete irrelevant info.

[nytai]

Ah, you're right it seems this code is checking supports_multivalues_insert which is part of the sqlalchemy api. Unfortunately it seems some dialect implementations aren't setting this property correctly.

There appears to be some inconsistency between the property on the db_engine_spec and this code, my guess is that it's a bug in the initial implementation or something didn't get cleaned up correctly during the refactor. I'd suggest changing the code to be something like if engine.dialect.supports_multivalues_insert or cls.supports_multivalues_insert:. This would allow superset to override this behavior if the db supports it but the dialect hasn't defined the property.

Here's an example of the snowflake driver not defining it until someone raised an issue snowflakedb/snowflake-sqlalchemy#55.

[sfirke]

That seems like a good fix. I can send it as a PR.

For posterity: I had been getting pymssql._mssql.MSSQLDatabaseException: (3902 ... error when performing a multi insert after enabling this behavior, it turns out that was isolated to the spreadsheet I had unfortunately picked at random. It had a value "1.3 5" with a space buried deep in a float column. When I cleaned up that data and resaved it, it uploaded. Also was able to upload it as a CSV. Not a problem with Superset, nor anything I'm interested in pursuing in openpyxl or sqlalchemy etc.