Downgrade thrift (again)

[anthonyroussel]

Hello :)

The usql build is failing with the all tag:

../../../go/pkg/mod/sqlflow.org/gohive@v0.0.0-20200521083454-ed52ee669b84/hiveserver2/gen-go/tcliservice/TCLIService.go:730:16: not enough arguments in call to iprot.ReadStructBegin
	have ()
	want (context.Context)
../../../go/pkg/mod/sqlflow.org/gohive@v0.0.0-20200521083454-ed52ee669b84/hiveserver2/gen-go/tcliservice/TCLIService.go:736:37: not enough arguments in call to iprot.ReadFieldBegin
	have ()
	want (context.Context)
[...]

This regression was introduced with commit 1f27d36 which upgrade Thrift from v0.13 to v0.14.2, see 1f27d36e

However since Thrift >= v0.13, there is a breaking change which was introduced, see apache/thrift@e79f764f09afd

And sqlflow.org/gohive needs to Thrift <= v0.13 to work.

A resolution is to downgrade thrift to v0.13 again:

$ go get github.com/apache/thrift@0.13.0
downgraded github.com/apache/thrift v0.15.0 => v0.13.1-0.20191017214740-b75e88a33d67
downgraded github.com/beltran/gohive v1.5.2 => v1.3.0

Because this problem already occured and was fixed with #206, I propose to build usql with the all tag during the test pipeline to detect this kind of regression more easily :)

[anthonyroussel]

I also opened a pull-request to gohive to upgrade the Thrift version to v0.16.0 and fix this issue definitively :)

See sql-machine-learning/gohive#70

I hope we can update sqlflow.org/gohive once this gohive's pull request is merged.

[kenshaw]

@anthonyroussel appreciate the work. Do you actually use this driver, or were you just testing with build -tags all?

[anthonyroussel]

@anthonyroussel appreciate the work. Do you actually use this driver, or were you just testing with build -tags all?

Thank you for your review! :)

No I don't use this driver.

My problem was that I could not update the usql package to v0.11 in nixpkgs because it builds usql with all the drivers by default:

• https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/applications/misc/usql/default.nix
• https://r.ryantm.com/log/usql/2022-04-10.log

[kenshaw]

If you're distributing usql, please don't build with all, as there are drivers that break output (ie, write logs to standard out and/or take over global logging, or have other unwanted side effects that don't play well with the other drivers) when imported.

I'd highly suggest using the set of tags within the build-release.sh:

    most    
    sqlite_app_armor    
    sqlite_fts5    
    sqlite_introspect    
    sqlite_json1    
    sqlite_stat4    
    sqlite_userauth    
    sqlite_vtable   
   no_adodb

I would even suggest using build-release.sh if you can within your build scripts.

[anthonyroussel]

If you're distributing usql, please don't build with all, as there are drivers that break output (ie, write logs to standard out and/or take over global logging, or have other unwanted side effects that don't play well with the other drivers) when imported.

I'd highly suggest using the set of tags within the build-release.sh:

    most    
    sqlite_app_armor    
    sqlite_fts5    
    sqlite_introspect    
    sqlite_json1    
    sqlite_stat4    
    sqlite_userauth    
    sqlite_vtable   
   no_adodb

I would even suggest using build-release.sh if you can within your build scripts.

Thanks for your advice! :)

I will check this to make sure that in nixpkgs we only use these tags to build usql.

Do you plan to release a v0.11.1 of usql soon?

[kenshaw]

@anthonyroussel likely this weekend if I have enough time. Unfortunately, I have to manually build the release artifacts on VMs because of the CGO dependencies. I was going to do it yesterday, but became busy with other development tasks.