astronomer / astro-sdk

Astro SDK allows rapid and clean development of {Extract, Load, Transform} workflows using Python and SQL, powered by Apache Airflow.
https://astro-sdk-python.rtfd.io/
Apache License 2.0
330 stars 40 forks source link

ExcelFileType class added #1978

Closed skolchin closed 11 months ago

skolchin commented 1 year ago

New file type class (ExcelFileType) added to support operations on Excel files (see https://github.com/astronomer/astro-sdk/issues/1956)

skolchin commented 11 months ago

LGTM. But look like there is a conflict and would be great if we can add a test here if possible https://github.com/astronomer/astro-sdk/blob/main/python-sdk/tests/sql/operators/test_load_file.py

@pankajastro , when I run pytest -s -v -x -k load_file in tests\sql, I'm getting an error astro.exceptions.NonExistentTableException: The table _tmp_xxx does not exist. Is this a problem you've mentioned?

pankajastro commented 11 months ago

@pankajastro , when I run pytest -s -v -x -k load_file in tests\sql, I'm getting an error astro.exceptions.NonExistentTableException: The table _tmp_xxx does not exist. Is this a problem you've mentioned?

Hi @skolchin, from log you have shared it is difficult to say why it happening. but test work on the local machine. We have some integration tests that require more setup like credentials etc.

I would suggest, maybe adding an airflow task here https://github.com/astronomer/astro-sdk/blob/main/python-sdk/example_dags/example_load_file.py would be great. We have everything in CI to run this

load_xlsv_file_in_postgres = aql.load_file(
        task_id="local_xlsv_to_postgress",
        input_file=File(path=str(CWD.parent) + "/tests/data/sample.xlsx", filetype=FileType.XLSX),
        output_table=Table(
            conn_id=ASTRO_POSTGRESS_CONN_ID,
        ),
    )
pankajastro commented 11 months ago

@skolchin do let us know if you need any support on this, looks like we are very close to merging it

skolchin commented 11 months ago

@skolchin do let us know if you need any support on this, looks like we are very close to merging it

@pankajastro Hi, sorry, was buzy yesterday. Test added and it seems it works

pankajastro commented 11 months ago

@pankajastro Hi, sorry, was buzy yesterday. Test added and it seems it works

Thanks, @skolchin, I'm looking into CI when it green I'll merge it

pankajastro commented 11 months ago

Look like some issue with the GitHub action. job is in queued state since long

pankajastro commented 11 months ago

This is awesome, thanks @skolchin 👏 for creating the PR. Appreciate it.

skolchin commented 11 months ago

@pankajastro @tatiana thank you!