ExcelFileType class added

[skolchin]

New file type class (ExcelFileType) added to support operations on Excel files (see #1956)

[tatiana]

Thank you very much for the contribution, @skolchin. It's looking great, including code, docs, and tests.

Minor comments in-line. I just enabled the tests, so we can confirm all the tests pass in the CI. Please, could you rebase this PR based on the latest main?

[pankajastro]

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

[skolchin]

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]

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

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

[skolchin]

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

LGTM

[utkarsharma2]

Looking good. Just one thing - can we change the dir structure? if it's not too much trouble?
it can be something like:
python-sdk/src/astro/files/types/excel
python-sdk/src/astro/files/types/excel/base.py
python-sdk/src/astro/files/types/excel/xls.py
python-sdk/src/astro/files/types/excel/xlsx.py

WDYT?

[pankajastro]

I thought about it but some test refactoring requires

[pankajastro]

maybe we can do it as separate PR if needed

[utkarsharma2]

suffix = "FileType"
yield method_map_fixture(
        method=method,
        classes=classes,
        base_path=base_path,
        get=lambda x: FileType(x[0:-len(suffix)].lower()),  # remove FileType suffix

Something like this would be better, compared to 0: -8. WDYT?

[tatiana]

Thanks for addressing all the feedback, @skolchin ! Looks great!

[pankajastro]

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

[skolchin]

@pankajastro @tatiana thank you!