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

Open
wants to merge 16 commits into
base: 2.0
Choose a base branch
from

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
@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
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