Fix joinGet error message

[spinojara]

Changelog category (leave one):

• Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Improve joinGet error message so that it properly states that the number of join_keys is not the same as the number of right_table_keys.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[george-larionov]

Hi @spinojara. First off, thanks for opening a PR! We love when people contribute to ClickHouse :).

The error message you have modified is not actually the full story. Take a look at this fiddle, you can see that the two error messages for the queries at the bottom are similar but not the same. For the top query, we get an error message that is correct and clearly does not match the code you edited. We don’t need to worry about this error message, since it is working fine, but it does give us a clue as to what’s going on.

The last query outputs the error message that you changed. So this error message is only hit when the arguments match the function signature, but the number of ‘right table’ key columns does not match the number of provided join_keys, otherwise the function will either work or hit some different error.

Please take a look at my other comments and make the necessary changes, let me know if you have any questions/concerns.

[george-larionov]

Also, please update the changelog entry in the PR comment to be a bit more descriptive.

[spinojara]

You are right, that makes it even clearer. I have updated the patch and the changelog entry. I still need to wait for Axis Communications to accept the CLA.

[clickhouse-gh]

Workflow [PR], commit [2563303]

Summary: ❌
15 failures out of 118 shown:

job_name	test_name	status	info	comment
Config Workflow		failure
	can_be_trusted	failure
Dockers Build (amd)		dropped
Dockers Build (arm)		dropped
Dockers Build (multiplatform manifest)		dropped
Style check		dropped
Docs check		dropped
Fast test		dropped
Build (arm_tidy)		dropped
Build (amd_debug)		dropped
Build (amd_release)		dropped
Build (amd_asan)		dropped
Build (amd_tsan)		dropped
Build (amd_msan)		dropped
Build (amd_ubsan)		dropped
Build (amd_binary)		dropped

[george-larionov]

You are right, that makes it even clearer. I have updated the patch and the changelog entry. I still need to wait for Axis Communications to accept the CLA.

It looks good now! Let's see what the tests say, although I believe they will fail until the CLA is signed/approved. See if you can either sign it again or there seems to be a link to 'recheck' it in the CLAassistant comment.

[george-larionov]

The changes look good to me now! Let me know if you continue to have issues with the CLA, I can look into it.

[AntonFriberg]

@george-larionov Sorry for the delay. @spinojara joined my team and was quick to contribute upstream which I am super happy about. However, it is the first time that our company has contributed to ClickHouse and our legal team needs to review the CLA in order for us to approve. I am doing everything I can to speed up the process. Sorry for the delay!

[george-larionov]

@george-larionov Sorry for the delay. @spinojara joined my team and was quick to contribute upstream which I am super happy about. However, it is the first time that our company has contributed to ClickHouse and our legal team needs to review the CLA in order for us to approve. I am doing everything I can to speed up the process. Sorry for the delay!

No problem Anton, there is no rush at all from our end!

[alexey-milovidov]

This change does not require CLA.

[spinojara]

Thanks!