search

ibis-project / ibis

the portable Python dataframe library
https://ibis-project.org
Apache License 2.0
4.43k stars 544 forks source link

Move `sql/sqlite` to `backends/sqlite` #2433

Closed baogianghoangvu closed 3 years ago

baogianghoangvu commented 3 years ago

Closes #2420

Please let me know if the PR needs any further additions. Thank you for reviewing!

baogianghoangvu commented 3 years ago

Thank you so much @datapythonista for your detailed review. I've made the fixes accordingly. Sorry for my mix-up in the tests.

baogianghoangvu commented 3 years ago

Thank you so much for your guidance. Hope I didn't take up too much of your time.

datapythonista commented 3 years ago

@baogianghoangvu CI problems should be fixed now. You'll have to merge master into your branch, and also change the module in https://github.com/ibis-project/ibis/blob/master/ci/recipe/meta.yaml#L64 to get the CI passing (ibis.sql.sqlite -> ibis.backends.sqlite).

baogianghoangvu commented 3 years ago

I've rebased my branch to the latest ibis master, and updated the ci recipe for sqlite.

datapythonista commented 3 years ago

Great, feel free to ping me if we get a green build. This should be ready. Thanks!

datapythonista commented 3 years ago

/azp run

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
baogianghoangvu commented 3 years ago

@datapythonista Hi! The previously failed check has succeeded. Though I'm not sure if the skipped and cancelled checks (impala-related) are expected or not.

datapythonista commented 3 years ago

/azp run

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
datapythonista commented 3 years ago

Not sure why conda has problems in the impala build, it worked well in the last PRs that we merged. I restarted the CI for now, but I'll have a look at the problem (builds timeout after one hour, and conda is that slow in that build).

datapythonista commented 3 years ago

/azp run

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
datapythonista commented 3 years ago

/azp run

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
datapythonista commented 3 years ago

@baogianghoangvu looks like CI is read because of isort. Can you merge master into your branch again, and run isort . in the root please.

Let's see if we get a green build and we can finally merge this. Thanks!

baogianghoangvu commented 3 years ago

When I isort ., some files from directories other than backends/sqlite are changed as well. The changes seem harmless so I'll try pushing.

Thank you for your advice!

baogianghoangvu commented 3 years ago

Looks like my local isort doesn't conform to the required config. I'm reverting the isort commit to see if this caused the PySpark tests to fail.

datapythonista commented 3 years ago

You'll have to update isort. conda env update should do it.

baogianghoangvu commented 3 years ago

That was my mistake. I ran isort from the wrong environment. I hope it's good now.

datapythonista commented 3 years ago

/azp run

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
jreback commented 3 years ago

thanks @baogianghoangvu

baogianghoangvu commented 3 years ago

Thank you @datapythonista for your guidance and review!

datapythonista commented 3 years ago

Thanks for your great work with this @baogianghoangvu. Feel free to work on other issues if you feel like it.