Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for doing binary encoding of result rows for clients that send that info (like PowerBI) #23

Merged
merged 9 commits into from
Oct 12, 2023

Conversation

jwills
Copy link
Owner

@jwills jwills commented Aug 12, 2023

I probably don't have the details of all of the different formats correct yet but will iterate on them here

@actuary87
Copy link

Hi,

I just noticed the new branch. I merged those changes into my fork under branch dev. I am getting index out of range errors around query_result.result_format[i] under send_row_description

Also due to the error, the Power BI connectivity is broken.

You can clone my fork (dev branch) and try out the example Power BI file. Should work out of the box (on main branch) or can be debugged (on dev).

@actuary87
Copy link

Wow. I am learning more of git now.

My dev branch brought all you commits in jwills_powerbi branch except the changes to requirements file.

The first commit in dev branch (I did it manually through vscode compare files and clicking the arrow to bring in your changes).

Other commits I cherry picked using git and I synced with github.

The out of range issue is still there!

@jwills
Copy link
Owner Author

jwills commented Aug 26, 2023

Ha-- love to see it, thanks so much for giving it a try!

Apologies for the lag on my side, I have a couple of other things in flight right now but will get back to this on Monday!

@actuary87
Copy link

No problem. Happy that you are interested too. First time I engage with someone on github.

Once this fixed, I guess Power BI + DuckDB (Direct Query) will be in a very good shape for production usage.

@jwills
Copy link
Owner Author

jwills commented Aug 29, 2023

@actuary87 okay I spent several hours on this today and I think I've got most of the issues sorted out; would you give it another crack when you have some time? 🙇

@actuary87
Copy link

Thanks a lot. I still get the errors. I will try to debug to understand.

I noted one thing, I hope it's a good sign: Although Power Query had errors (unable to connect to db) but when I opened Power BI today the cache version of the table surprisingly showed integer numbers.

If it means something it means somehow it was able to parse binary format before the error!

I hope so.

Will give it a debug try and let you know.

@actuary87
Copy link

actuary87 commented Sep 25, 2023

Thank you very much for the changes. I did some of debugging during the weekend and I managed to solve the errors I reported earlier. So your changes works well, but I need to test all data types before I fully conclude.

Power Query don't report errors now :D

With regards to Numeric data, Integers are ok but Decimal does not work unless I send it as TEXT (changing OID to 25 for both Decimal and Unknown types).

It'll be good if I know how Postgresql sends the Decimal to the client. I can't seem to find the proper document which explains this. Do you have something to share in this regard?

I will push changes in my fork and merge them to main once the testing goes well.

@jwills
Copy link
Owner Author

jwills commented Sep 25, 2023

Hey @actuary87, always a pleasure to hear from you! Let me see if I can figure out the Decimal encoding format, it would obviously be amazing if that is all that is holding us back now!

@jwills jwills merged commit 57c7359 into main Oct 12, 2023
3 checks passed
@jwills
Copy link
Owner Author

jwills commented Oct 12, 2023

Going to merge this for now to get it out there; getting the decimal binary protocol correct is going to be some work/take some time to get it right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants