Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/Microsoft.Data.Analysis/DataFrame.Join.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ public partial class DataFrame
private void SetSuffixForDuplicatedColumnNames(DataFrame dataFrame, DataFrameColumn column, string leftSuffix, string rightSuffix)
{
int index = dataFrame._columnCollection.IndexOf(column.Name);

// The loop below appends leftSuffix to the existing column and rightSuffix to the new one until the two
// names differ. If both suffixes are the same the names stay equal however many times they are appended,
// so there is no name that ends the loop.
if (index != -1 && leftSuffix == rightSuffix)
{
throw new ArgumentException(string.Format(Strings.SuffixesMustBeDifferent, nameof(leftSuffix), nameof(rightSuffix), column.Name), nameof(rightSuffix));
}

while (index != -1)
{
// Pre-existing column. Change name
Expand Down
9 changes: 9 additions & 0 deletions src/Microsoft.Data.Analysis/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Microsoft.Data.Analysis/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@
<data name="StreamDoesntSupportReading" xml:space="preserve">
<value>Stream doesn't support reading</value>
</data>
<data name="SuffixesMustBeDifferent" xml:space="preserve">
<value>{0} and {1} must be different when both DataFrames contain a column called '{2}'</value>
</data>
<data name="ValueNameAlreadyExists" xml:space="preserve">
<value>Value name '{0}' matches an existing column name</value>
</data>
Expand Down
34 changes: 34 additions & 0 deletions test/Microsoft.Data.Analysis.Tests/DataFrameTests.Join.cs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,40 @@ public void TestJoin()
VerifyJoin(join, left, right, JoinAlgorithm.Inner);
}

[Theory]
[InlineData(JoinAlgorithm.Left)]
[InlineData(JoinAlgorithm.Right)]
[InlineData(JoinAlgorithm.FullOuter)]
[InlineData(JoinAlgorithm.Inner)]
public void TestJoin_SameSuffixOnBothSides_Issue6128(JoinAlgorithm joinAlgorithm)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public void TestJoin_SameSuffixOnBothSides_Issue6128(JoinAlgorithm joinAlgorithm)
public void TestJoin_ColumnNamesCollide_ThrowsOnSameSuffixOnBothSides(JoinAlgorithm joinAlgorithm)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

renamed in 45d2f6f . gave the merge test the same name shape so the pair reads alike, TestMerge_ColumnNamesCollide_ThrowsOnSameSuffixOnBothSides.

{
DataFrame left = MakeDataFrameWithNumericColumns(3, false);
DataFrame right = MakeDataFrameWithNumericColumns(4, false);

// Both frames have a column called "Int", and one suffix cannot make the two names different,
// so the join used to keep renaming for ever instead of coming back
Assert.Throws<ArgumentException>(() => left.Join(right, "_same", "_same", joinAlgorithm));
Assert.Throws<ArgumentException>(() => left.Join(right, "", "", joinAlgorithm));

// different suffixes still work
DataFrame join = left.Join(right, "_left", "_right", joinAlgorithm);
Assert.Equal(left.Columns.Count + right.Columns.Count, join.Columns.Count);
}

[Fact]
public void TestJoin_SameSuffixWithNoSharedColumnNames()
{
DataFrame left = new DataFrame(new Int32DataFrameColumn("Left", new int?[] { 0, 1, 2 }));
DataFrame right = new DataFrame(new Int32DataFrameColumn("Right", new int?[] { 0, 1, 2 }));

// no name is shared, so nothing is renamed and the suffixes are never used
DataFrame join = left.Join(right, "_same", "_same");

Assert.Equal(2, join.Columns.Count);
Assert.Equal("Left", join.Columns[0].Name);
Assert.Equal("Right", join.Columns[1].Name);
}

private void VerifyJoin(DataFrame join, DataFrame left, DataFrame right, JoinAlgorithm joinAlgorithm)
{
Int64DataFrameColumn mapIndices = new Int64DataFrameColumn("map", join.Rows.Count);
Expand Down
13 changes: 13 additions & 0 deletions test/Microsoft.Data.Analysis.Tests/DataFrameTests.Merge.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,19 @@ public void TestMerge_CorrectColumnTypes()
Assert.NotNull(merge.Columns.GetDateTimeColumn("DateTime_right"));
}

[Fact]
//Issue 6128

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
//Issue 6128

I get the idea and it might be useful when consistent, but we don't relate tests like this usually.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

dropped in 45d2f6f, thanks.

public void TestMerge_SameSuffixOnBothSides()
{
DataFrame left = MakeDataFrameWithNumericColumns(3, false);
DataFrame right = MakeDataFrameWithNumericColumns(4, false);

Assert.Throws<ArgumentException>(() => left.Merge<int>(right, "Int", "Int", "_same", "_same"));

DataFrame merge = left.Merge<int>(right, "Int", "Int");
Assert.Equal(left.Columns.Count + right.Columns.Count, merge.Columns.Count);
}

private void VerifyMerge(DataFrame merge, DataFrame left, DataFrame right, JoinAlgorithm joinAlgorithm)
{
if (joinAlgorithm == JoinAlgorithm.Left || joinAlgorithm == JoinAlgorithm.Inner)
Expand Down
Loading