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

🐛 [BUGFIX] For SAPRFCV2 adding whitespaces to match sub - queries #1100

Merged
merged 20 commits into from
Nov 26, 2024

Conversation

marcinpurtak
Copy link
Collaborator

@marcinpurtak marcinpurtak commented Nov 4, 2024

Summary

Seems like SAP returns columns with different amount of whitespaces - ideally SAP server should always add whitespaces , that the amount of characters matches the declared data type in SAP table definition.
However, after some investigation , looks like from every n-th SAP request, it is trimming all whitespaces or messing the number of added ones - which could lead to wrongly merged SAP responses using unique columns as the merge key.

In this PR there is a solution, that, for each UNIQUE columns, checks the returned SAP table metadata to obtain what is the declared data type ( and the characters amount ) , then it checks what is the received character amount and adds whitespaces to match the declared one.

Whitespaces are added using vector/matrix approach which doesn't slow down the ingestion process ( not a huge impact on performance).

This PR closes #556

Importance

Important because missing whitespaces could lead to problems with joining sub - queries based on the unique columns

Checklist

This PR:

  • follows the guidelines laid out in CONTRIBUTING.md
  • links relevant issue(s)
  • adds/updates tests (if appropriate)
  • adds/updates docstrings (if appropriate)
  • adds an entry in CHANGELOG.md

@marcinpurtak marcinpurtak added bug Something isn't working 2.0 viadot 2.0 labels Nov 4, 2024
@marcinpurtak marcinpurtak marked this pull request as ready for review November 5, 2024 10:39
@marcinpurtak marcinpurtak requested a review from trymzet November 5, 2024 10:39
@m-paz
Copy link
Contributor

m-paz commented Nov 6, 2024

hi @trymzet
It's a bugfix, there is a milestone for 2025 to refactor the whole connector but as of now we need to continue with this one.

Thanks!

Copy link
Contributor

@trymzet trymzet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added feedback, can you pls also add at least 1 unit test for this?

src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
src/viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved
@marcinpurtak marcinpurtak requested a review from trymzet November 21, 2024 14:48
@m-paz m-paz requested review from m-paz and removed request for m-paz November 26, 2024 08:49
@m-paz m-paz self-requested a review November 26, 2024 08:49
@marcinpurtak marcinpurtak merged commit 587a7fc into 2.0 Nov 26, 2024
3 checks passed
@marcinpurtak marcinpurtak deleted the saprfcv2-adding-whitespaces branch November 26, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 viadot 2.0 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants